DBAL-669 - Update SQL generated by the schema tool will also create schemas

[Ocramius]

DBAL-669 - Updating a schema should introduce also schema creation statements in the generated SQL

[Ocramius]

@chrisramakers ping

[beberlei]

@Ocramius What about Create SQL Listener? Does he create the schema as well?

[beberlei]

This is not the correct approach, as we don't create schemas as well when doing doctrine:schema:create. Schemas are outside the scope of the DBAL Schema component (yes, weird naming issue). You have to create them manually. Same goes for databases.

[beberlei]

Hm, it seems we have changed that some time ago, reopening to evaluate

[Ocramius]

@beberlei we had some schema creation support in platforms in master since @ea0254731d42c94ccd9ebfba48f72adbafbe5232

dbal/lib/Doctrine/DBAL/Schema/Visitor/CreateSchemaSqlCollector.php

Lines 142 to 147 in ea02547

	foreach (array_keys($this->createTableQueries) as $namespace) {
	if ($this->platform->supportsSchemas() && $this->platform->schemaNeedsCreation($namespace)) {
	$query = $this->platform->getCreateSchemaSQL($namespace);
	array_push($sql, $query);
	}
	}

2.4.1 does not have this

[beberlei]

Problem in this PR:

• SchemaDiff#$newNamespaces is not really true, we need a AbstractSchemaManager#listNamespaceNames() method and put them onto Schema instances.
• Duplicates are not handled correctly.

[deeky666]

@beberlei I think this is a good idea. I was very confused lately about namespaces in table objects. If you retrieve a table from database with schema manager you get different objects depending on whether you specify the full qualified table name including namespace or not.

// Schema manager
$qualifiedTable   = $sm->listTableDetails('namespace.table');
$unqualifiedTable = $sm->listTableDetails('table'); // automatically uses platform's default namespace

$qualifiedTable->getNamespaceName(); // returns "namespace"
$unqualifiedTable->getNamespaceName() // returns null

$this->assertEquals($qualifiedTable, $unqualifiedTable); // false

IMO the above comparison should be true. I don't know the background of why it was implemented this way.

[chrisramakers]

@beberlei @Ocramius How does this patch solve DBAL-669? The hardcoded schema names in the postgresqlPlatform file continue to be an issue since these "existing" schema's should be retrieved from the live database, not based on hardcoded strings ... https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php#L659-L665

[Ocramius]

@chrisramakers can those schemas actually be changed? (didn't check)

[chrisramakers]

What do you mean? Those schema's don't need to be changed. The schemaNeedsCreation method is used to check if a given schema exists in the database. The problem is that the method doesn't really check the database if the schema's exist but just returns true in case the schema name is neither "default" or "public".

If I'd call this method with "data" as argument $this->schemaNeedsCreation("data") and the "data" schema isn't present in my database, how is this method ever going to return true when it's just not checking the database but just blatantly returning info based on an hardcoded array of schema names?

What needs to be done here imho is replace the schemaNeedsCreation method body with code that actually checks if the passed $schemaName argument is an existing schema in the postgresql database. Getting a list of existing schema names from the database is a single query

SELECT nspname FROM pg_catalog.pg_namespace;

PS: there might be some naming confusion between a database schema describing tables and a postgresql schema being a container for tables, views, ...

[Ocramius]

@chrisramakers I was asking if postgres schemas default and public can actually be changed or if they are hardcoded/enforced by postgres itself (besides other schemas).

You're right on this PR not solving the problem of DDC-669, where different schemas are being created, but the schema only does introspection on the tables in the default/public schema afaik (doesn't check other schemas for table existence), so the PR should at least generate the correct diff.

I'll mark the PR as WIP since it obviously isn't ready for merging.

[mvrhov]

@Ocramius: at least for public I can say no it's not enforced. You can freely delete it.

[chrisramakers]

the public schema is created by default but it can be deleted so you can't rely on it being there.

[deeky666]

@Ocramius We have to implement the namespaces introspection suggested by @beberlei in order for the schema tool to calculate the differences in namespaces correctly. Also IMO schemaNeedsCreation() has to be removed again as it serves not purpose when not reliable. This is in theory the same as if having a tableNeedsCreation() method which tries to exclude possible system tables. This should not be part of the platform IMO.

[deeky666]

@Ocramius @chrisramakers Okay the implementation is finished now from my side. Can you please review.

[Ocramius]

@beberlei you mentioned that the "Visitor" is used here. Is it such a big deal that we cannot simply add the NamespaceVisitor to the Visitor interface instead? Wouldn't a BC break here be ok?

[stof]

adding new methods in an interface is a BC break, as classes implementing the interface now throw a fatal error because of the missing method

[Ocramius]

Yes, and I was saying that this interface is not commonly used...

[chrisramakers]

@Ocramius @stof @beberlei Any progress on this issue? It's been a dealbreaker for almost a year now and still no fix.

http://www.doctrine-project.org/jira/browse/DBAL-669

[Ocramius]

Needs a rebase, but IMO it is good to be merged. @deeky666 thoughts?

[deeky666]

@Ocramius yep it can be merged IMO. Do I have to rebase?

[Ocramius]

@deeky666 if you can, then I'll merge

[chrisramakers]

Follow up on this one, the fix is in use now but the problem stays the same.

In our application we use the Doctrine\DBAL\Schema() class to define a new table that will be created by our tools. The sql queries returned by Schema::toSql() contain a "CREATE SCHEMA" (read namespace) statement, regardless of whether the namespace exists or not. Since postgres doesn't have a "CREATE SCHEMA IF NOT EXISTS" option the resulting sql queries from the Schema::toSql() method will always fail on the first query which just throws a postgres error "Schema already exists"

What should be done is use the connection to first check if the namespace exists, if not add the "CREATE NAMESPACE" query, else only add the appropriate table queries.

[deeky666]

@chrisramakers I think you are just using the API wrong. Schema::toSql() uses the CreateSchemaSqlCollector to generate the SQL. In fact that API is not intended for updates but for creating a fresh schema instead assuming none of the assets are present in the DB. This API is decoupled from the schema manager and therefore from schema introspection.
I assume what you want to do is compare an offline and online schema, create a schema diff and then generate SQL from the schema differences. Like the following:

// declare $platform
// declare $schemaManager

$offlineSchema = new \Doctrine\DBAL\Schema\Schema();

// declare $offlineSchema

$onlineSchema = $schemaManager->createSchema();

$comparator = new \Doctrine\DBAL\Schema\Comparator();
$schemaDiff = $comparator->compare($onlineSchema, $offlineSchema);

if ($schemaDiff) {
    $sql = $schemaDiff->toSql($platform);
}

// execute SQL

[chrisramakers]

@deeky666 the thing is that i'm not updating any schema's. The app we built requires us to create tables for certain entities (not doctrine entities) in our app. For example when a new "Car"-type is added a "Cars" table is created (very simplistic).

This is a slightly altered snippet of the code we currently use

<?php
$schema = new Schema();
$table = $schema->createTable($tableName);
$table->addColumn("asset_id", "integer");
$table->addColumn($columnName, $columnType);
$sql = $schema->toSql($this->em->getConnection()->getDatabasePlatform());
foreach($sql as $query){
    $this->conn->exec($query);
}

I think the problem is related to the fact i'm not using the schemaManager to create the table but rather execute the raw queries. I've just tried and used the active SchemaManager from the DBAL Connection to create the table and the code below seems to work as expected.

<?php
$table = new Table($tableName);
$table->addColumn("asset_id", "integer");
$table->addColumn($fieldName, $dataType, $dataTypeOptions);
$table->addColumn($columnName, $columnType);
$this->em->getConnection()->getSchemaManager()->createTable($table);

[deeky666]

@chrisramakers yes you use the wrong API for your use-case. The second approach is the proper one. If you just want to create a single table, use the schema manager. Schema::toSql() is really only intended to create a whole new schema.