Rename table instruction for postgres uses quoteColumnName

[MasterOdin]

Something I noticed while fixing #1851, but did not have time to investigate on writing a testcase for it, so making an issue so as to not lose track of it.

The getRenameTableInstructions for the postgres adapter has the following definition:

https://github.com/cakephp/phinx/blob/1e1597112b919bbe9c864e2d00f2ee7227b36613/src/Phinx/Db/Adapter/PostgresAdapter.php#L352-L362

On line 358, we are using quoteColumnName instead of quoteTableName. This will probably end up doing something unexpected if using a table name with schema. A test should probably be written to verify if that instruction breaks under current conditions and is fixed once that instruction is changed to quoteTableName.

[MasterOdin]

Looking at this further, I think there's a question on what the expected behavior of a user when they do something like:

$this->adapter->renameTable('schema1.table1', 'schema2.table1');

Where, right now, this gets translated into the SQL statement of ALTER TABLE "schema1"."table1" RENAME TO "schema2.table1" which ends up with then having the end result of "schema1"."schema2.table". Given that other commands (and the first argument of this function) split on the schema, it feels reasonable to assume you would end up with the SQL statement of ALTER TABLE "schema1"."table1" RENAME TO "schema2"."table1", which would then throw an error as one cannot have a schema used in the second part of the rename. Instead, it would have to do ALTER TABLE "schema1"."table1" SET SCHEMA "schema2";, with a follow-up table rename if necessary.

My view of available options are then:

1. Leave it as is, maybe write a note in the docs for user beware
2. Change it to use the above quoteTableName and have it throw an error for invalid syntax
3. Rework the whole function such that if schema($tableName) !== schema($newTableName), issue first a SET SCHEMA statement, then issue a RENAME.

Thoughts? I suppose something to also note is that it doesn't seem like anyone's yet this or at least hit and filed a bug so it's unclear how much of an issue this really is out in the wild.

[othercorey]

I think we should only use the table name without schema in the ALTER TABLE query.

If users are specifying an invalid schema for the target, we could error/warn before executing.

[MasterOdin]

I think we should only use the table name without schema in the ALTER TABLE query.

Yes, you have to. Otherwise postgres will throw an error.

If users are specifying an invalid schema for the target, we could error/warn before executing.

I'm not sure what you mean here. Are you saying that doing $this->adapter->renameTable('schema1.table1', 'schema1.table2'); should work as it should recognize that the two schemas are the same, and so silently drop using it from the second argument, while doing $this->adapter->renameTable('schema1.table1', 'schema2.table1'); should error?

A follow-up question I would have is the expectation for a user who might want to rename / move a table into another schema, and should phinx provide some sort of support for this, or is it on the user to craft the raw SQL to do this?

[othercorey]

Are you saying that doing $this->adapter->renameTable('schema1.table1', 'schema1.table2'); should work as it should recognize that the two schemas are the same, and so silently drop using it from the second argument, while doing $this->adapter->renameTable('schema1.table1', 'schema2.table1'); should error?

I think so.

A follow-up question I would have is the expectation for a user who might want to rename / move a table into another schema, and should phinx provide some sort of support for this, or is it on the user to craft the raw SQL to do this?

I think phinx could have a changeSchema that calls ALTER TABLE name SET SCHEMA schema. Does phinx have a way to create schemas already? If not, should this be creating a schema if it doesn't exist?

I'm not sure it should be silently incorporated into renameTable as that could be dangerous since it's uncommon.

[MasterOdin]

I think phinx could have a changeSchema that calls ALTER TABLE name SET SCHEMA schema. Does phinx have a way to create schemas already? If not, should this be creating a schema if it doesn't exist?

Phinx does not have any direct methods for creating / deleting schemas, nor a changeSchema method. I think schema support is a somewhat "here be dragons" type situations within phinx.

e: I was mistaken, there are within the postgres adapter the following methods:

• createSchema
• hasSchema
• dropSchema
• dropAllSchemas
• getAllSchemas

but no method to transfer a table from one schema to another. It would probably be worth examining bringing these methods to other adapters that support schemas (e.g. sqlite?).

[othercorey]

I would limit this to validating schema name and fixing the rename. Full schema support would need a design.

[MasterOdin]

Yup, got a patch cooking where it only fixes the rename in the case where the schema on the second argument matches the schema of the first argument. For example, the following would happen:

renameTable('table1', 'public.table2') -> ALTER TABLE "public"."table1" RENAME TO "table2" (where public is the default schema) renameTable('schema1.table1', 'table2') -> ALTER TABLE "schema1"."table1" RENAME TO "table2" renameTable('schema1.table1', 'schema1.table2') -> ALTER TABLE "schema1"."table1" RENAME TO "table2" renameTable('schema1.table1', 'schema2.table2') -> throw new \InvalidArgumentException('Cannot rename table into different schema');

And then as you indicate, further design consideration can be put into supporting schemas and such.