Migration generates invalid SQL for dropping unique constraints on PostgreSQL #86

Open
horros opened this Issue Sep 3, 2011 · 19 comments

Comments

Projects
None yet
5 participants
@horros
Contributor

horros commented Sep 3, 2011

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

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand Sep 3, 2011

Member

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.

Member

willdurand commented Sep 3, 2011

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

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 3, 2011

Contributor

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?

Contributor

horros commented Sep 3, 2011

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

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand Sep 4, 2011

Member

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

Member

willdurand commented Sep 4, 2011

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

@fzaninotto

This comment has been minimized.

Show comment Hide comment
@fzaninotto

fzaninotto Sep 5, 2011

Member

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

Member

fzaninotto commented Sep 5, 2011

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

@nnarhinen

This comment has been minimized.

Show comment Hide comment
@nnarhinen

nnarhinen Sep 5, 2011

Contributor

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.

Contributor

nnarhinen commented Sep 5, 2011

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

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 5, 2011

Contributor

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.

Contributor

horros commented Sep 5, 2011

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

This comment has been minimized.

Show comment Hide comment
@nnarhinen

nnarhinen Sep 5, 2011

Contributor

@horros, absolutely, tomorrow will be fine

Contributor

nnarhinen commented Sep 5, 2011

@horros, absolutely, tomorrow will be fine

@willdurand

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand Sep 6, 2011

Member

Thanks guys to take time to improve this part :)

Member

willdurand commented Sep 6, 2011

Thanks guys to take time to improve this part :)

@horros

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 6, 2011

Contributor

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?

Contributor

horros commented Sep 6, 2011

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

This comment has been minimized.

Show comment Hide comment
@nnarhinen

nnarhinen Sep 6, 2011

Contributor

+1 for isConstraint.

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

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

Contributor

nnarhinen commented Sep 6, 2011

+1 for isConstraint.

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

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

@horros

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 6, 2011

Contributor

+1 from me too, would be MUCH cleaner.

Contributor

horros commented Sep 6, 2011

+1 from me too, would be MUCH cleaner.

@fzaninotto

This comment has been minimized.

Show comment Hide comment
@fzaninotto

fzaninotto Sep 6, 2011

Member

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?

Member

fzaninotto commented Sep 6, 2011

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

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 6, 2011

Contributor

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).

Contributor

horros commented Sep 6, 2011

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

This comment has been minimized.

Show comment Hide comment
@nnarhinen

nnarhinen Sep 6, 2011

Contributor

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.

Contributor

nnarhinen commented Sep 6, 2011

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 pushed a commit to horros/Propel that referenced this issue Sep 22, 2011

@horros

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 22, 2011

Contributor

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.

Contributor

horros commented Sep 22, 2011

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

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand Sep 22, 2011

Member

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

Member

willdurand commented Sep 22, 2011

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

@horros

This comment has been minimized.

Show comment Hide comment
@horros

horros Sep 23, 2011

Contributor

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

Contributor

horros commented Sep 23, 2011

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

@willdurand

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand Sep 23, 2011

Member

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:
#86 (comment)

Member

willdurand commented Sep 23, 2011

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:
#86 (comment)

@rayrigam

This comment has been minimized.

Show comment Hide comment
@rayrigam

rayrigam Sep 2, 2013

Contributor

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");
Contributor

rayrigam commented Sep 2, 2013

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");

willdurand added a commit that referenced this issue Sep 9, 2013

Merge pull request #86 from c33s/master
added favicon.ico.psd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment