Generate alter add/change column with explicit position for MySQL #4276

mvorisek · 2020-09-19T19:06:52Z

Q	A
Type	feature
BC Break	no
Fixed issues	Alter add/change column now has now explicit position, so new/updated column is updated to the same position like when the table is created freshly. - doctrine/orm#8258 - doctrine/migrations#558 - https://stackoverflow.com/questions/34873327/doctrine-migration-add-column-after-other-one - https://stackoverflow.com/questions/24148568/doctrineschemaupdate-doesnt-respect-column-order

morozov · 2020-09-20T23:12:06Z

Thank you for the patch, @mvorisek. Before proceeding, please make sure the new logic is covered with a functional test that demonstrates the desired end result.

mvorisek · 2020-09-22T22:12:59Z

Thank you for the patch, @mvorisek. Before proceeding, please make sure the new logic is covered with a functional test that demonstrates the desired end result.

@morozov functional test added

morozov · 2020-09-22T22:19:57Z

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

+        }
+
+        if (! $columnFound) {
+            throw new InvalidArgumentException('Column name "' . $columnName . '" not found');


Would it be possible to avoid having this exception by using a more suitable method signature? Each column is either first or goes after some other column. See #4145 for example. Otherwise, this will need a functional test that demonstrates the exception being thrown.

we can call $toTable->getColumn() before the foreach loop which should check if the column exists or not

please sudgest ideal approach to this

It might work if instead of having the $toTable in the SchemaDiff, the ColumnDiff had a property like $afterColumn that would contain the column name and be non-empty only if the column position has changed. This assumes that a similar SQL syntax could be used on other supported platforms. This would also solve the problem of having the FIRST and AFTER fragments unnecessarily generated.

ColumnDiff can not be used as we need position also for Column. @morozov I would like to finish this PR asap, please advise, if the $toTable is ok - used like $fromTable already used - or if you have a better alternative. Thanks.

ColumnDiff can not be used as we need position also for Column

I do not understand what this means. Can you please elaborate?

ColumnDiff can not be used as we need position also for Column

I do not understand what this means. Can you please elaborate?

if column already exists, then ColumnDiff instance is returned from the comparator

if the column is new (not renamed/modified), then there is no ColumnDiff for it

exception is covered by added test

@mvorisek please make sure you don't resolve the conversations that you didn't start unless you're resolving one on the starter's behalf.

morozov · 2020-09-22T22:22:49Z

tests/Doctrine/Tests/DBAL/Functional/Schema/MySqlSchemaManagerTest.php

@@ -260,7 +286,7 @@ public function testColumnCharsetChange(): void
        $diff = $fromSchema->getMigrateToSql($toSchema, $this->connection->getDatabasePlatform());
        self::assertContains(
            'ALTER TABLE test_column_charset_change CHANGE col_string'
-                . ' col_string VARCHAR(100) CHARACTER SET ascii NOT NULL',
+                . ' col_string VARCHAR(100) CHARACTER SET ascii NOT NULL FIRST',


Why do we expect the SQL to contain the FIRST/AFTER qualifiers if the column position doesn't change? I believe these should be only generated when necessary.

we generate this for ALTER TABLE only, for TABLE CREATE, position is not generated

if position is the same, we regenerate whole column definition

Can we generate the column position only if it's changed?

I can add a condition, if the previous column is the same for fromTable and toTable, then to not add the position. Tests may however need more changes as currently, we expect that SQL column definition is always fully regenerated.

Let solve this as the last step before merge, firstly, I want to make sure toTable property will stay

I think toTable should stay since the other solution with ColumnDiff instance will not handle the case of new columns, which means we can resume this conversation. Let's generate the position only when necessary please.

Let's generate the position only when necessary please.

I understand we want to generate minimalistic diff, but I think we do not want it. ALTER TABLE ADD/MODIFY column is always generated fully even if there is a minor column definition change like column comment. Adding position based on the original table will violate this definition.

@morozov @greg0ire can we agree on this and can we merge this PR?

can we agree on this

If it's always generated fully indeed then it's ok with me.

can we merge this PR

In theory yes, in practice the tests fail

tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php

morozov · 2020-09-23T01:52:59Z

As long as this new feature requires breaking or non-breaking API changes, I'd like to make sure that the new API could be used to implement the same feature for other platforms. Otherwise, it may require a BC break in the future.

@mvorisek could you do some research and see which SQL syntax could be used for the rest of the platforms to implement this in the future?

mvorisek · 2020-09-23T07:07:28Z

As long as this new feature requires breaking or non-breaking API changes, I'd like to make sure that the new API could be used to implement the same feature for other platforms. Otherwise, it may require a BC break in the future.

the proposed data structure (with $toTable - there is already simillar use of $fromTable) we can implement it everywhere

could you do some research and see which SQL syntax could be used for the rest of the platforms to implement this in the future?

I already did, other DB platforms require table recreation, ie. create table, insert into select, drop table

morozov · 2020-09-25T03:25:52Z

I already did, other DB platforms require table recreation, ie. create table, insert into select, drop table

In this case, I'm not sure we should be adding this feature.

mvorisek · 2020-09-25T11:18:56Z

I already did, other DB platforms require table recreation, ie. create table, insert into select, drop table

In this case, I'm not sure we should be adding this feature.

This feature - reflecting column order defined by Table - has been asked by many users and I think we should do as much as we can to support it.

I think:

for MySQL, the proposed impl. is very clean and it does allow MySQL users to update DB like if it was created freshly
for other DBs, this feature can be implemented by recreate and reimport - eg. this PR lands a concept for it and someone else can implement this feature for other engines

@morozov can we agree on this and merge?

wdyt about adding public function supportsColumnRerder(): bool into AbstractPlatform, so user can check easily if reorder is supported or not? (and once supported in another platform, this method can be simply overrided in BC fashion)

morozov · 2020-09-25T14:34:06Z

This feature - reflecting column order defined by Table - has been asked by many users and I think we should do as much as we can to support it.

What's the use case for this? When does the order of columns in a table matter to the application that uses it?

for MySQL, the proposed impl. is very clean

I disagree. I expressed my concerns above.

for other DBs, this feature can be implemented by recreate and reimport - eg. this PR lands a concept for it and someone else can implement this feature for other engines

This is not completely true. We cannot introduce a new API that works for one DB platform and expect it to work for others that knowingly don't support the way of implementing the feature. That's why I asked to check how other platforms behave in this regard.

wdyt about adding public function supportsColumnRerder(): bool into AbstractPlatform

It's not the best idea. Every time a "supports" method is added to the API, it forces the platform-specific logic to be implemented outside of the platform. It invalidates the purpose of the API which is to encapsulate the details of each its implementation behind a common interface.

so user can check easily if reorder is supported or not

What if it's not supported? If the code can handle this case for other platforms, why can't it handle for MySQL?

For now, this could be reliably used in the code:

if ($platform instanceof MySQLPlatform) {
    // reordering supported
}

mvorisek · 2020-09-25T17:24:50Z

This feature - reflecting column order defined by Table - has been asked by many users and I think we should do as much as we can to support it.

What's the use case for this? When does the order of columns in a table matter to the application that uses it?

Basically any applications that use the database/column order like:

any end user app that fetches/display data using select *
database administration/development tools

the later point is very important from developers UX perspective and this PR improves it hugely, as column order is now like when a all tables would be created freshly - good for visual orientation, diffs, ...

for MySQL, the proposed impl. is very clean

I disagree. I expressed my concerns above.

I know, please reply to #4276 (comment) , I will then solve the impl. according to that, I think, toTable is clean approach and it goes well with the exisitng fromTable. If have a better idea, please propose, ColumnDiff can not be used as column may be new and Column itself is not the right place to contain afterColumn as Column as managing this data structure will be much harder instead of "just ordered list" like now

for other DBs, this feature can be implemented by recreate and reimport - eg. this PR lands a concept for it and someone else can implement this feature for other engines

This is not completely true. We cannot introduce a new API that works for one DB platform and expect it to work for others that knowingly don't support the way of implementing the feature. That's why I asked to check how other platforms behave in this regard.

now I propose for MySQL only, later, see above, it can be implemented with multiple SQL queries like "RENAME TABLE", "CREATE TABLE", "INSERT INTO SELECT", "DROP TABLE"

morozov · 2020-09-29T02:01:13Z

The link to #4276 (comment) doesn't take me anywhere. Could you please quote your question?

database administration/development tools

IMO it's an edge case that we can potentially support in a subset of supported platforms. Given how unclear the current approach is, it's not a priority for me personally at the moment. I'm fine if somebody else from the project team works on this and helps solve the issues I identified above.

mvorisek · 2020-10-07T12:00:30Z

@greg0ire can you please review and provide your feedback?

greg0ire · 2020-10-10T16:09:01Z

This feature has been asked by many users and I think we should do as much as we can to support it.

Can you please link to Github tickets in the "fixed issues" row of the table in your original message?

mvorisek · 2020-10-10T16:30:16Z

This feature has been asked by many users and I think we should do as much as we can to support it.

Can you please link to Github tickets in the "fixed issues" row of the table in your original message?

Added. From my side, this PR is complete, but if you have any ideas, I am ready to implement them so we can release this asap. Thanks.

greg0ire · 2020-10-10T16:40:11Z

From my side, this PR is complete

Otherwise, this will need a functional test that demonstrates the exception being thrown.

I think you still have to address this, both codecov and Sergei mentioned it.

mvorisek · 2020-10-16T09:29:09Z

@greg0ire I fixed merge conflict, please rereview and please include this in the next upcoming 2.11 release.

mvorisek · 2020-10-22T12:35:39Z

all feedback has been fixed, can we merge now?

morozov

@greg0ire what's your take on whether this behavior should be added in the first place? See #4276 (comment).

morozov · 2020-10-22T14:32:50Z

tests/Doctrine/Tests/DBAL/Functional/Schema/MySqlSchemaManagerTest.php

+
+        $comparator    = new Comparator();
+        $diff          = $comparator->diffTable($table, $table2);
+        $diff->toTable = $table2WithoutAColumn;


Once the diff is generated from the two tables, why should it be possible to change the referenced table via the API?

this is test - to test exception https://github.com/doctrine/dbal/pull/4276/files#diff-548b552a74ea52eab68318520c213a142ba77527a4b965c4d3bd50eeba1afa9aR675

This test was meant to be added to document the case when this exception can happen because IMO it shouldn't be possible in the first place (https://github.com/doctrine/dbal/pull/4276/files#r493065750). The test doesn't show a valid case. Which means that there is no justification for this API to throw an exception.

When the diff object is built using Comparator::diff which will add the input, then this exception can not happen.

However, the toTable is public property - like fromTable already present - and if the table is replaced by the user with different table that has not all the column names set - this exception is legit and this test cover it.

greg0ire · 2020-10-22T15:22:16Z

@greg0ire what's your take on whether this behavior should be added in the first place? See #4276 (comment).

I think you know far more than I do about the philosophy of this project, and if you're not sure, I'm even less sure than you are… maybe @beberlei can advise on this?

@beberlei, is it commonplace to accept PRs that add features that can never be implemented on other platforms?

SenseException · 2020-10-22T19:57:31Z

I already did, other DB platforms require table recreation, ie. create table, insert into select, drop table

In this case, I'm not sure we should be adding this feature.

We have features that are not supported by other platforms, but might be added at some point. Here we can't implement it to certain platforms because there a recreation is needed. I'm not really sold yet to improve one platform that might be the only one with this feature, because the mentioned benefits aren't big ones.

While doing some research I found something in the PostgreSQL docs that has one technical benefit mentioned in http://wiki.postgresql.org/wiki/Alter_column_position:

physical layout can be optimized by putting fixed size columns at the start of the table

But I'm not sure if this is also the same benefit in MySQL.

mvorisek · 2020-11-03T16:17:30Z

@greg0ire please reaprove, I rebased it from 2.11.x to 2.12.x branch.

physical layout can be optimized by putting fixed size columns at the start of the table

But I'm not sure if this is also the same benefit in MySQL.

see https://stackoverflow.com/questions/894522/is-there-any-reason-to-worry-about-the-column-order-in-a-table - there is some performance improvement with controlled columns placement

Is there anything I can do from my side to get this PR merged?

mvorisek · 2020-11-05T13:03:27Z

solved privately, contact me in PM if you are a company and want to pay to open source it, more work needed than this PR

mvorisek force-pushed the mysql_alter_column_with_position branch 6 times, most recently from c2b4179 to 354a914 Compare September 19, 2020 22:08

morozov added Improvement Schema Management MySQL labels Sep 20, 2020

mvorisek force-pushed the mysql_alter_column_with_position branch 5 times, most recently from 48c5d0c to f9e6b05 Compare September 21, 2020 09:44

morozov reviewed Sep 22, 2020

View reviewed changes

mvorisek force-pushed the mysql_alter_column_with_position branch 2 times, most recently from 91056a7 to 9348097 Compare October 12, 2020 22:01

greg0ire requested review from SenseException and ostrolucky October 14, 2020 18:47

mvorisek dismissed greg0ire’s stale review via 36df46a October 16, 2020 09:13

greg0ire previously approved these changes Oct 16, 2020

View reviewed changes

morozov reviewed Oct 22, 2020

View reviewed changes

mvorisek added 4 commits October 27, 2020 19:56

Generate alter add/change column with explicit position for MySQL

ed88b44

update tests

7a68d23

improve exception message

51aed61

update mysql test

a460898

mvorisek dismissed greg0ire’s stale review via 2355a3a October 27, 2020 18:57

mvorisek force-pushed the mysql_alter_column_with_position branch from 263ce35 to 2355a3a Compare October 27, 2020 18:57

mvorisek changed the base branch from 2.11.x to 2.12.x October 27, 2020 18:57

mvorisek force-pushed the mysql_alter_column_with_position branch 4 times, most recently from 7c4bbec to d491ef2 Compare October 27, 2020 19:15

test explicit column position in alter table

962efa9

mvorisek force-pushed the mysql_alter_column_with_position branch from d491ef2 to f0a78df Compare October 27, 2020 19:24

cover column not found exception

e6408e2

mvorisek force-pushed the mysql_alter_column_with_position branch from f0a78df to e6408e2 Compare October 27, 2020 19:24

mvorisek closed this Nov 5, 2020

mvorisek deleted the mysql_alter_column_with_position branch November 5, 2020 13:03

morozov mentioned this pull request Apr 3, 2021

Fix identifier escaping when used as string literal for T-SQL #4360

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022

Generate alter add/change column with explicit position for MySQL #4276

Generate alter add/change column with explicit position for MySQL #4276

Conversation

mvorisek commented Sep 19, 2020 • edited Loading

morozov commented Sep 20, 2020

mvorisek commented Sep 22, 2020

morozov Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Sep 23, 2020

mvorisek commented Sep 23, 2020

morozov commented Sep 25, 2020

mvorisek commented Sep 25, 2020

morozov commented Sep 25, 2020

mvorisek commented Sep 25, 2020

morozov commented Sep 29, 2020

mvorisek commented Oct 7, 2020

greg0ire commented Oct 10, 2020

mvorisek commented Oct 10, 2020

greg0ire commented Oct 10, 2020

mvorisek commented Oct 16, 2020

mvorisek commented Oct 22, 2020

morozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Oct 22, 2020

SenseException commented Oct 22, 2020

mvorisek commented Nov 3, 2020 • edited Loading

mvorisek commented Nov 5, 2020

mvorisek commented Sep 19, 2020 •

edited

Loading

morozov Sep 22, 2020 •

edited

Loading

morozov Sep 22, 2020 •

edited

Loading

mvorisek commented Nov 3, 2020 •

edited

Loading