Migration generates invalid SQL for dropping unique constraints on PostgreSQL

[horros]

If we have a table in the schema and add a <unique> element, eg

<unique>
  <unique-column name="foo" />
</unique>

the migration target generates

CREATE UNIQUE INDEX "test_U_1" on "test" ("foo");

which is fine, except if we remove the unique-element, the generated migration code from PgsqlPlatform::getDropIndexDDL() is

ALTER TABLE "test" DROP CONSTRAINT "test_U_1";

this won't work, because the unique index created is a, well, unique index, not a table or column constraint.

There are two (maybe three) possible fixes for this. Either the generator needs to create the add SQL along the lines of

ALTER TABLE "test" ADD CONSTRAINT "test_U_1" UNIQUE ("foo");

or generate drop SQL along the lines of

DROP INDEX "test_U_1";

The third option I suppose is poke around in the metadata-tables in PostgreSQL and figure out if it's a table/column constraint and call some getDropUniqueConstraintDDL() or something similar, but that may quckly become way too hard to do.

Maybe the schema (and Propel) should differentiate between unique indexes and table/column constraints somehow? Maybe the <vendor>-element could be used for this?

I'm not quite sure what the real-world difference between a table constraint and a unique index on a column really is, other than you can't drop the constraint with DROP INDEX because the constraint depends on the index, and you can't drop the index with ALTER TABLE .. DROP CONSTRAINT, becuase it isn't a table constraint.

[willdurand]

Hi, I think the second option is ok (about to drop the index with DROP INDEX ...).

Would you write a patch for that ? The difference between unique indexes and table constraints is another point and, as the migration task creates a UNIQUE INDEX we have to drop it.

[horros]

Can do. Hmm, once I get it done, should I add a new pull request or do some magic and make the changes appear in this issue?

[willdurand]

You can add a link to this PR in your commit message by adding something like that: Fixes #86.

[fzaninotto]

Since you're modifying code introduced by @nnarhinen in the first place, I'd like to have his opinion about this. Niklas?

[nnarhinen]

IIRC the reason why I checked for Unique in the first place, is because PgsqlPlatform has the following

public function getUniqueDDL(Unique $unique)
{
    return sprintf('CONSTRAINT %s UNIQUE (%s)',
        $this->quoteIdentifier($unique->getName()),
        $this->getColumnListDDL($unique->getColumns())
    );
}

and thus the unique index actually has to be dropped as a table constraint.

[horros]

The generator does generate CREATE UNIQUE INDEX "test_U_1" ON "test" ("test"); for me when adding a unique index to the schema file, so there must be something wrong somewhere else then. I could not drop the index that the generator created with the drop code that the generator generated. @nnarhinen, do you have time to have a look at this with me tomorrow or so? Something fishy seems to be going on.

[nnarhinen]

@horros, absolutely, tomorrow will be fine

[willdurand]

Thanks guys to take time to improve this part :)

[horros]

I almost have this working like it should.

The problem was that the SQL-target generates SQL-files with

CREATE TABLE "table" (...., CONSTRAINT "foo" UNIQUE ("bar"))

but the migration task generates

CREATE UNIQUE INDEX "foo" ON "table" ("bar");

Since the SQL task generates constraints, the migration task now also assumes constraints, but those can be forced off (ie. generate "normal" unique indexes) by using

<unique>
  <unique-column name="foo" />
  <vendor type="pgsql">
    <parameter name="constraint" value="false" />
  </vendor>
</unique>

PgsqSchemaParser properly detects if the index in the database belongs to a constraint or if it's just a normal index, and sets the VendorInfo-object accordingly.

The only problem I have now is if the unique in the database (or schema) has been a constraint and is changed to a "normal" index (or vice versa) I've no way of knowing that in PropelIndexComparator unless I

1. add an isConstraint() or similar method to Unique.php, or
2. use the VendorInfo-object in the comparator, which feels really meh.

I'm afraid I'm not at all familiar with how the other RDBMSes handle unique indexes/constraints, so I'm not really sure what route would be the most appropriate. Suggestions and comments?

[nnarhinen]

+1 for isConstraint.

<unique isConstraint="true">
...
</unique>

with a sane default for other vendors seems fine and consistent to me.

[horros]

+1 from me too, would be MUCH cleaner.

[fzaninotto]

Using VendorInfo in the comparator must be avoided as much as possible.

Using a tag in the constraint seems fine, but why to you need Unique::isConstraint() in that case? Is it just a proxy method to the embedded vendor object?

[horros]

If we don't use Unique::isConstraint() or some similar flag, then one needs to use VendorInfo in PropelIndexComparator. There isn't really any other way of knowing if the index belongs to a constraint or not.

The method could be a proxy to the related VendorInfo-object, but since XmlToAppData has access to PropelPlatformInterface one could very well imagine it encountering a unique-tag and setting the constraint-flag for it accordingly. It would also make PgsqlSchemaParser prettier :) On the other hand, using both the vendor-tag and some isConstraint()-flag could probably be used to implement support for CHECK constraints too later on if such support is deemed necessary.

Having an isConstraint()-method would also make PropelIndexComparator more consistent, eg.

if ($fromIndex->isConstraint() != $toIndex->isConstraint()) {
    return true;
}

instead of fetching the vendor info of both indexes, checking if they have the parameter, take into account the default value, comparing them etc (which we agree should be avoided as far as possible).

[nnarhinen]

yeah, the problem here is, that even though we would know if the index is unique (by reverse engineering and by processing schema.xml) we have to be able to check while creating the migration code, for instance the correct DROP clause for the constraint.

Also IMO one has to be able to compare if the unique index is initialized the same way in both schema.xml and in the reverse engineered table. That is, you have to be able to compare if it is a table constraint or a "normal" index. You might want to change (for some reason) in schema.xml from one to another type.

[horros]

Sorry it took me so long, been mad busy and then I caught a terrible cold.

Anyway, what the commit does is pretty much what I outlined before with extra checks and fixes for things that would not work. Does not interfere in any way with any other platform than PostgreSQL, and should now generate drop and create properly for both unique indexes and unique constraints.

Comments? Suggestions? Am I completely off track here?

Mind you, PHPUnit generated completely nonsensical code coverage reports when I ran the tests, but I think I have all the changed stuff tested.

Edit: Also apparently stuffed something when squashing and rebasing and whatnot, the timestamp is completely off. Sorry about that.

[willdurand]

No problem, could you create a PR ? It will be better for reviewing your work.

[horros]

Sure. Err, do I just open a new pull request or can I somehow attach a pull request to this current issue?

[willdurand]

Open a new PR. If a commit message says something like "Fixes #86", so it will be ok.

Envoyé de mon iPhone

Le 23 sept. 2011 à 08:57, Markus Lervikreply@reply.github.com a écrit :

Sure. Err, do I just open a new pull request or can I somehow attach a pull request to this current issue?

Reply to this email directly or view it on GitHub: https://github.com/propelorm/Propel/issues/86#issuecomment-2175906

[rayrigam]

This seems to still be an issue in the current 1.7.*@dev version. I just tried to do a Migration for adding a unique index and constraint to my PostgreSql database by adding the following to my schema:

    <unique>
      <unique-column name="foo"/>
    </unique>

After running the "propel:migration:generate-diff" command, I get the following "up" code in my PropelMigration file:

public function getUpSQL()
{
    return array (
        'default' => '
            CREATE UNIQUE INDEX "organization_U_4" ON "organization" ("url");
        ',
    );
}

The above getUpSQL only generates the new index in the database. When adding the unique tag to the schema, a unique constraint should also be created. Shouldn't it? I'm not sure if I fully get the discussion above, but it seems like the proper Migration code should be (as indicated above):

ALTER TABLE "test" ADD CONSTRAINT "test_U_1" UNIQUE ("foo");