Rolling back columns and indices added in separate migrations clashes with index recreation

[ndm2]

When a column with an index is being added in a separate migration, this will, when being rolled back, cause an error when the column is being dropped, as the fix from #2040 will try to re-add the index for which the referenced column(s) doesn't exist anymore.

I mistakenly assumed that reversed plans would drop indices before removing columns, however while the inverted commands do indeed generate index drops for the to be removed columns, these index drops are being filtered out by the plans conflict resolution:

phinx/src/Phinx/Db/Plan/Plan.php

Lines 193 to 204 in 3cfd71a

	// Columns that are dropped will automatically cause the indexes to be dropped as well
	foreach ($this->columnRemoves as $columnRemove) {
	foreach ($columnRemove->getActions() as $action) {
	if ($action instanceof RemoveColumn) {
	[$this->indexes] = $this->forgetDropIndex(
	$action->getTable(),
	[$action->getColumn()->getName()],
	$this->indexes
	);
	}
	}
	}

---

The error can be reproduced using these two migrations, where table creation comes first, and then table alteration. Rolling back the alteration migration will trigger the error.

== 20221014120113 RollbackFailureAlter: reverting

In PdoAdapter.php line 192:

SQLSTATE[HY000]: General error: 1 no such column: foo_id

<?php
declare(strict_types=1);

use Phinx\Migration\AbstractMigration;

final class RollbackFailureCreate extends AbstractMigration
{
    public function change()
    {
        $this
            ->table('test')
            ->create();
    }
}

<?php
declare(strict_types=1);

use Phinx\Migration\AbstractMigration;

final class RollbackFailureAlter extends AbstractMigration
{
    public function change()
    {
        $this
            ->table('test')
            ->addColumn('foo_id', 'integer')
            ->addIndex(['foo_id'], ['name' => 'FOO_ID_IDX'])
            ->update();
    }
}

[MasterOdin]

Ugh, yeah, now I more greatly remember why I wrote #2040 (comment) in the first place and marked the PR as a draft 🤦

I think a similar bug may occur then on renaming a column that has an index on it?

[ndm2]

Not sure if renaming a column is affected too.

As far I as I understand it, renaming a column only renames the column, so that it would require people to explicitly drop indices beforehand, and re-add them afterwards, as otherwise they could already run into errors because of dependencies of the index?

That would then require them to use up() and down(), so reversed actions wouldn't be involved.

[ndm2]

Well after a quick test it seems that this is only true for certain indices depending on the DBMS, there are indices for which the column references will be renamed accordingly too when renaming a column.

So I guess renaming columns would be affected too 😢

[ndm2]

Thinking about it, it is known what column are being renamed/removed, so shouldn't it be possible to parse the index creation SQL and compare it against the change, and then either exclude the index in case one or more of its columns are being dropped, or rename its columns accordingly?

[MasterOdin]

Yes, and it's probable that can do this via a simple regex as I'd expect the SQL we pull out of schema_master to be somewhat well-formed, but would have to validate that.

[ndm2]

Given that they can be created manually, and support expressions, there can probably be quite a variety in syntax. But I guess it would be reasonable to expect that people who are using fancy custom indices, to then handle updating them themselves when columns are being renamed/dropped, ie Phinx would only need to support the kind of indices syntax that can be created through its own API?

[ndm2]

So, I looked into this a bit, tried a bunch of different SQLite versions, and it seems that the rule is that almost everything after CREATE [UNIQUE] INDEX is basically (syntactically correct) garbage in, garbage out. For example:

CREate INdex test_idx On test (foo asc , aBs(bar)   DEsc )

is stored as-is, except for CREate INdex, meaning sqlite_master will contain/return:

CREATE INDEX test_idx On test (foo asc , aBs(bar)   DEsc )

I also looked into the behavior of other DBMS for renaming/dropping columns with indices, and it varies. MySQL will error out if any column of the index is a function expression, Postgres drops indices and renames columns even when function expressions are involved (not sure if there are edge cases), and in SqlServer it seems that using expressions isn't possible, dropping columns seems to always trigger an error, and renaming columns seems to give some kind of notice (which I assume needs to be obtained manually, I'd need to check on that again).

Long story short, there is no consistent behavior that the SQLite adapter would necessarily have to consider.

Unless you're already working on something @MasterOdin, I can give it a shot.

[ndm2]

Checking SqlServer again, it actually has a similar problem as Sqlite here, on rollback it will trigger an error because that's what SqlServer seems to do when dropping columns that have dependent indices.

== 20221014120113 RollbackFailureAlter: reverting

In PdoAdapter.php line 192:

SQLSTATE[42000]: [Microsoft][ODBC Driver 18 for SQL Server][SQL Server]The index 'FOO_ID_IDX' is dependent on column 'foo_id'.

@MasterOdin Do you remember whether filtering out the index drop actions from the plan was actually required to solve a specific problem? If it was just an optimization, then maybe it can be removed.

[MasterOdin]

It is almost certainly just an optimization, and there's probably no harm in doing a drop index before dropping a column during a migration, other than a few more queries run. However, I don't think that people really demand their migration library runs the absolute minimal set of queries, and are probably fine with a bit of non-optimization for correctness.

If we really wanted to keep it, we could just do an adapter type check that wraps around that block that checks if this optimization is supported or not.

[MasterOdin]

For the rename case, we should create a separate issue to track that and related discussion, and keep this focused just on the rollback bug in the OP which sounds like a much more straight-forward fix.

[ndm2]

I'll create a new issue for the renaming problems, and probably also one for explicit column dropping, as the implications of the implicit dropping as part of a rollback, are a bit different. For explicit drops the question is whether/when to automatically drop indices for SQLite.

[MasterOdin]

For explicit drops the question is whether/when to automatically drop indices for SQLite.

I would see if we can just remove that optimization for now and if there's nothing breaking about it, I'd go with that for now.

[ndm2]

I would classify this as two different problems, removing the optimization will not affect a removeColumn() that one would explicitly invoke in a migration, and since it is kind of "affected" by the index restoration change, I figured that it might still be worth looking into it in time.