Added existence check before dropping constraints in postgres #365

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

stefk commented Aug 28, 2013

The Schema object generates distinct sql queries for altering a table and for removing a constraint or an index. However, in some cases, the drop query may come after an alter table query deleting the column to which the index/constraint was bound. In postgres, where dropping a column automatically drops its attached indexes and constraints, executing the queries in that order leads to an sql error (trying to drop something that has already been deleted -> "SQLSTATE[42704]: Undefined object").

This PR introduces a quick fix for that issue (not sure it's the best way though).

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

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

We use Jira to track the state of pull requests and the versions they got
included in.

Owner

beberlei commented Sep 8, 2013

@stefk How does this happen? The DropSchemaSqlCollector drops all constraints first.

Contributor

stefk commented Sep 8, 2013

@beberlei, the problem doesn't happen when I drop the whole schema, but when I use the Schema#getMigrateToSql and Schema#getMigrateToSql to generate migration queries. The process goes fine 99% of the time (including all typ)

@stefk stefk closed this Sep 8, 2013

@stefk stefk reopened this Sep 8, 2013

Contributor

stefk commented Sep 8, 2013

(oops... suite)
The process goes fine 99% of the time (including all types of schema modifications), but sometimes this issue raises. It seems to be related to self-reference constraints (e.g a foreign key pointing on another column of the same table).

Contributor

stefk commented Sep 8, 2013

@beberlei, I wasn't sure if the problem didn't come for the "custom" code I use for generation, so I managed to reproduce the issue using only the DoctrineMigrationsBundle.

On an empty database and having the following entity:

<?php

namespace Foo\BarBundle\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity
 */
class Baz
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
     * @ORM\ManyToOne(
     *     targetEntity="Foo\BarBundle\Entity\Baz",
     *     inversedBy="id"
     * )
     * @ORM\JoinColumn(nullable=false, onDelete="CASCADE")
     */
    private $parent;
}

the doctrine:migrations:diff command will output, as expected:

<?php

namespace Application\Migrations;

use Doctrine\DBAL\Migrations\AbstractMigration,
    Doctrine\DBAL\Schema\Schema;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
class Version20130908164512 extends AbstractMigration
{
    public function up(Schema $schema)
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() != "postgresql", "Migration can only be executed safely on 'postgresql'.");

        $this->addSql("CREATE SEQUENCE Baz_id_seq INCREMENT BY 1 MINVALUE 1 START 1");
        $this->addSql("CREATE TABLE Baz (id INT NOT NULL, parent_id INT NOT NULL, PRIMARY KEY(id))");
        $this->addSql("CREATE INDEX IDX_40694278727ACA70 ON Baz (parent_id)");
        $this->addSql("ALTER TABLE Baz ADD CONSTRAINT FK_40694278727ACA70 FOREIGN KEY (parent_id) REFERENCES Baz (id) ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE");
    }

    public function down(Schema $schema)
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() != "postgresql", "Migration can only be executed safely on 'postgresql'.");

        $this->addSql("ALTER TABLE Baz DROP CONSTRAINT FK_40694278727ACA70");
        $this->addSql("DROP SEQUENCE Baz_id_seq CASCADE");
        $this->addSql("DROP TABLE Baz");
    }
}

Now if the previous migration has been executed and if the attribute referencing the entity itself is removed:

<?php

namespace Foo\BarBundle\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity
 */
class Baz
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;
}

the diff will be:

<?php

namespace Application\Migrations;

use Doctrine\DBAL\Migrations\AbstractMigration,
    Doctrine\DBAL\Schema\Schema;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
class Version20130908164829 extends AbstractMigration
{
    public function up(Schema $schema)
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() != "postgresql", "Migration can only be executed safely on 'postgresql'.");

        $this->addSql("ALTER TABLE baz DROP parent_id");
        $this->addSql("ALTER TABLE baz DROP CONSTRAINT fk_40694278727aca70");
        $this->addSql("DROP INDEX idx_40694278727aca70");
    }

    public function down(Schema $schema)
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() != "postgresql", "Migration can only be executed safely on 'postgresql'.");

        $this->addSql("ALTER TABLE Baz ADD parent_id INT NOT NULL");
        $this->addSql("ALTER TABLE Baz ADD CONSTRAINT fk_40694278727aca70 FOREIGN KEY (parent_id) REFERENCES baz (id) ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE");
        $this->addSql("CREATE INDEX idx_40694278727aca70 ON Baz (parent_id)");
    }
}

As you can see in the up method, the column is dropped before the fk and the index. Executing this migration will produce the sql error mentionned above.

Note: this only happens in postgres, not mysql (I haven't tested other rdbms so far).

+ {
+ $sql = parent::getDropIndexSQL($index, $table);
+
+ return str_replace('DROP INDEX', 'DROP INDEX IF EXISTS', $sql);
@deeky666

deeky666 Dec 20, 2013

Member

This is a misuse of the API. See the discussion here. I think the right solution for this is to patch the method for altering the table in the platform. The execution order should be fixed so that indexes and constraints will get dropped before altering columns. This line seems to be the problem. I think adopting MySQL's logic should fix it.

@beberlei

beberlei Dec 20, 2013

Owner

@deeky666 I agree, can you draft a new PR?

@deeky666

deeky666 Dec 20, 2013

Member

@beberlei I will do on sunday probably.

+ {
+ $sql = parent::getDropConstraintSQL($constraint, $table);
+
+ return str_replace('DROP CONSTRAINT', 'DROP CONSTRAINT IF EXISTS', $sql);
@deeky666

deeky666 Dec 20, 2013

Member

See previous comment.

Owner

beberlei commented Dec 21, 2013

Fixed for master and 2.4.2 in a different way: 8f94c5d

@beberlei beberlei closed this Dec 21, 2013

@stefk stefk deleted the stefk:postgres-constraints branch Jan 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment