Problem/Motivation
- DBTNG does not support adding a serial field to an existing table, even if all the supported DB engines can do that just fine
- DBTNG does not handle primary keys properly when adding a new field to an existing table. \Drupal\Core\Database\Schema::changeField()
documents that primary keys have to be handled manually by the calling code, but \Drupal\Core\Database\Schema::addField()
documents that it handles primary keys by itself.
Proposed resolution
Fix DBTNG to support adding a serial field to an existing table.
Fix implementations of \Drupal\Core\Database\Schema::addField()
to handle primary key changes automatically.
Remaining tasks
Review.
User interface changes
Nope.
API changes
Nope.
Data model changes
Nope.
Comments
Comment #2
amcgowanca CreditAttribution: amcgowanca commentedI ran into the same issue when updating the Backup & Migrate module to the latest version.
Comment #3
djdevinThis is an issue with MySQL 5.7, trying to add a new serial field will result in:
SQLSTATE[42000]: Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead
Does not happen with MySQL <= 5.6
Using an int field works, but then you have to manually make it auto-increment.
Applying the patch made it work.
Similar to #2665362: D7 upgrades fail with mysql 5.7:
Example of an update that fails on 5.7: http://cgit.drupalcode.org/quiz/tree/quiz.install#n877
Comment #4
gregglesI ran into this issue as well related to the login_history module.
The patch in #2 fixes it for me.
Thanks, @amcgowanca!
Comment #5
gregglesThis actually seems RTBC to me. I'm about to have a contrib release that depends on it and would rather have a hook_requirements that looks for an up to date core than instructing people to install a patch.
Comment #6
joseph.olstad+1 by @rudiedirkx
#2428371: Upgrade from 1.3 to newer (1.4, 1.5, 3.x) with drush fails.
see comments
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer commentedLooks good to me, is D8 affected, too?
In Drupal coding standards we use:
$spec['type'] != 'serial'
--
Also this needs a comment to explain why we want to check this.
Else someone else might be tempted to remove this in the future.
Setting to CNW for the missing comment.
Comment #8
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #9
joseph.olstadBased on suggestion by Fabianx, here is the D8 version of this patch
To follow is the updated D7 version.
Comment #10
joseph.olstadComment #11
joseph.olstadComment #12
joseph.olstadComment #14
joseph.olstadComment #16
joseph.olstadkick the testbot until this passes.
D8 patch passed all tests, I expect both patch 13 (d7/d8) to pass as well and if they don't keep kicking them until they do.
I prefer Patches 13 , I think (based on my interpretation of the code) should be better performance than the previous ones.
Comment #17
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC for #9 then in Drupal 8.
Comment #18
joseph.olstadSuggest #13 (14) as preferred. Although I have not profiled performance it is likely more efficient than #9. Looks prettier because 13/14 is in original pre-patch order
Thx
Comment #19
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC for #14 is okay with me, too.
Order should not matter at all as there is no function calls, but I am fine either way.
Comment #21
djdevinStill an issue and patch applies cleanly to 8.4.x-dev.
7.x patch applies cleanly too.
Unfortunately testbot doesn't have 5.7, but it does fail and the patch does fix it.
Comment #22
alexpottDiscussed with @catch, @cilefen, @cottser, @laurii, and @xjm. We agreed that this is a major issue as there is no work around and this is part of another major bug - #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field
Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe problem is not only about serial fields, but all added fields that will be part of the table's primary key. And it's not limited to the MySQL driver, Postgres has the same problem.
Here's the relevant parts from the patch in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.
Comment #25
jibranThis is reviewed and tested extensively in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field so coming from there I'm setting it to RTBC.
Comment #26
alexpottShouldn't we have some assertion that the primary key is as expected?
Comment #27
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSure we can. Let's try this.
Comment #28
timmillwood#27 looks good.
Comment #29
timmillwoodah, just saw the test, not so good got postgres and sqlite.
Comment #30
tstoecklerThis comment is reversed, right? I.e. fields that are part of a primary key *must* be NOT NULL, i.e. cannot be added as not NOT NULL ?!
Comment #31
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@tstoeckler, you're right, I was going for a double negation but that's always hard when 'not null' is involved.
It's so fun finding bugs like this in DBTNG after so many years :)
Comment #32
lokapujyaRan a test by recreating a table and using the API to add a primary key.
Comment #33
alexpottSometimes I feel as though we should ban array addition. So many bugs.
Hmmm this is interesting. We're expanding the scope here to increase the public API of the SQLite driver. Rather than changing this here can we not just do the query on the database like we do for MYSQL and PSQL?
Comment #34
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@alexpott, the reason why I chose to make that method public is that we can't run this query
'PRAGMA ' . $info['schema'] . '.table_info(' . $info['table'] . ')'
from the outside. Specifically, we don't have access to the table prefixes which are internal to the SQLite schema driver (i.e. \Drupal\Core\Database\Schema::getPrefixInfo() is protected).Also,
table_info()
is not a real table, so we can't use\Drupal\Core\Database\Driver\sqlite\Connection::getFullQualifiedTableName()
(which is already a public method) on it.Maybe your concern could be addressed by marking it @internal?
Comment #35
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAs discussed with @alexpott on IRC, let's use reflection instead of making that method public.
Comment #36
jibranEven better. RTBC +1.
Comment #38
alexpottBugs fixed in all 3 core supported database drivers and new test coverage. Nice work!
Updating issue credit.
Comment #39
alexpottCommitted and pushed 45d8336 to 8.4.x and de15524 to 8.3.x. Thanks!
I've backported this to 8.3.x because it is a major self-contained bug with no API changes. Setting to patch to be ported so the D7 issue can be created.
Comment #42
joseph.olstadBackport is RTBC ready AFAIK
https://www.drupal.org/files/issues/d7-mysql_schema_add_field_serial_typ...
Comment #43
joseph.olstadRTBC +1
Tested this with mysql 5.7.x , we require this core patch for upgrading workbench_moderation from 1.3.x to 3.x.
Without patch :
Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead
With patch:
Works without errors.
I recommend the D7 patch be targetted for next release.
adjusting tags, it's already been committed to D8, and the D7 patch has been ready for a long time so it doesn't need a backport.
Commit for the win.
Comment #44
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch that was committed to 8.x (#35) is *very* different than the latest 7.x patch (#14), so someone needs to actually write a 7.x version of the patch in #35 before this can be RTBC.
Comment #45
joseph.olstad@amateescu , ok, new D7 patch to follow shortly
Comment #46
joseph.olstadSee what the testbot says.
Comment #48
joseph.olstadsyntax issue
need to rework array syntax for php 5.3 compatibility
New patch to follow shortly
Comment #49
joseph.olstadNew patch with interdiff
Comment #51
joseph.olstadComment #52
joseph.olstadChanging from assertSame to assertEqual fixed it on my local environment, I expect patch 51 to pass the mysql tests, not sure about the others like Postgres or sqlite because I don't have a test environment for those.
Comment #53
joseph.olstadhmm, not sure why the postgresql test is failing, perhaps head is also failing.
Comment #54
joseph.olstadComment #55
joseph.olstadpatch 51 is a valid D7 backport of what was committed to D8.
Test results on Postgres and sqlite are tainted by what appears to be jenkins configuration issues (and or head issues) with the test postgres/sqlite environments and db abstraction layers.
Comment #56
joseph.olstadComment #57
joseph.olstadSuggest we commit the MySql part of this patch which we are 100% confident with and postpone the sqlite and postgresql portion until head passes for those respective db systems.
Easily whip up another patch .
Recommend that we move forward quickly on this.
Comment #58
tstoeckler@joseph.olstad Please do not mark your own patch as RTBC. Even if it is just a backport, someone should verify that and also we need to review that it properly fixes the problem in D7.
Comment #59
joseph.olstad@tstoeclker , please provide a reason why this is NOT RTBC.
Comment #60
joseph.olstad(postgres) Possibly related issues:
#998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL
#2487269: Postgres insert queries that fail in a transaction break the entire transaction
#1013034: PostgreSQL constraints do not get renamed by db_rename_table()
(sqlite) possibly related issues:
#2427773: SQLite REGEXP user function exists, but is wrongly implemented.
#1713332: The SQLite database driver fails to drop simpletest tables
#2427875: SQLite Insert query does not account for INSERT FROM ... SELECT.
Comment #61
joseph.olstadNew patch, making change to sqlite schema.inc after having a closer look at the comment #33 from alexpott above
see interdiff and new patch
Comment #62
joseph.olstadremoved commented out line that had no place in the patch. For test results, refer to patch 61. See interdiff for details.
Comment #63
joseph.olstadsame as patch 62 , re-uploading because it looks like the tests on patch 61 stopped.
Comment #64
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis change is not correct. @alexpott argued for the exact opposite thing, that we should not make this protected method public.
Comment #65
joseph.olstad@amateescu
Yes definately, here's a new patch.
Comment #66
joseph.olstadIMHO rtbc this backport. As for postgresql and sqlite, there seems to be a problem with the Testbot /docker config or Jenkins for those. Test results often inconsistent, does not reflect upon the quality of this backport. At very least, fast track the MySql commit, it is needed for MySql 5.7.x and higher. Currently Testbot is only 5.5.x.
Comment #67
mlncn CreditAttribution: mlncn at Agaric for Drutopia commentedTested D7-2615496-65.patch, albeit MySQL only. Good to go.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThere were some minor issues in the tests (for PHP 5.2 and Drupal 7 compatibility) which I've fixed in the attached. Will run this against all 3 databases and we can compare it to https://www.drupal.org/node/1970534#comment-12115998
Overall, the patch looks good, but:
I am not entirely sure why we want to make this change (vs. simply documenting that you have to explicitly call db_drop_primary_key() first, which in practice has always been the case).
It seems unrelated to the original bugfix here. And I am not totally convinced that it is good to have this kind of magic. db_add_field() is about adding fields... so why should it magically delete primary keys rather than simply failing (like it currently does) if you try to add an illegal field? If a developer adds a new field with a primary key under the mistaken belief that the table doesn't have a primary key yet, shouldn't they be notified of that rather that just having the primary key silently switched?
The old behavior was clearly broken, but I'm not convinced array_merge() is correct here either. Suppose a table has multiple 'indexes'. array_merge() will remove all the old indexes and only use the ones passed in via db_add_field(). That means if you are adding a new field and adding an index for it, you also have to remember to pass in every existing index (for other fields in the same table) or SQLite will delete them? That is not consistent with how it works in MySQL, for example, which only requires that you pass in any indexes that you're changing (and leaves other existing ones alone).
So I suspect the correct code is more like
$new_schema['indexes'] = array_merge($new_schema['indexes'], $keys_new['indexes'])
and similar for the other types of keys?At least this is worth a followup issue, for Drupal 8 also. I'm not sure if it's worth skipping the above change in Drupal 7 until this is addressed (depends on which behavior we think is more broken).
Overall, I think this should be held until Drupal 7.60 since the fixes are a bit risky and it could use more review.
Limiting the patch to just be about the broken NULL handling for primary keys (on MySQL and PostgreSQL) could also help get it in faster.
Comment #69
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #71
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat's because of composite primary keys :) When you have a table that contains a serial field (which is by default part of the primary key), you can not drop the existing primary key of that table before adding the new field. This exact scenario is now tested so it's quite easy to check for yourself locally, just comment out that code in
addField()
and try to runSchemaTestCase
on MySQL.I think you're right, but wouldn't it be simpler to just do a nested merge instead?
Comment #72
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #73
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNot sure how much we can learn from those test failures overall, but one thing is that some of the SQLite failures are real and caused by this patch. That's because:
There is no
$result
- it should probably be something like$table_info['primary key']
instead.Regarding #71:
You're right, it's not always as simple as just calling db_drop_primary_key(), but I think it still can be done. To remove the primary key from a serial field before adding a new one, I can still get the tests to pass (at least on MySQL) by adding this:
(and then presumably a db_drop_unique_key() afterwards once the new primary key is created)
So I guess it is a bit more annoying than I thought in that particular case, but it's no different than what you would have to do with db_change_field() (I think?). So my overall point still stands.
I think that would be wrong in the case of some particular array elements. And in fact, the suggestion I had above is actually wrong for the same reasons...
For example if
$new_schema['primary key'] = array('column1')
and$keys_new['primary key'] = array('column2')
we actually wantarray('column2')
as the result. But either my suggestion or yours would givearray('column1', 'column2')
in that case.So the code for this would actually need to be a bit more careful.
Comment #74
MustangGB CreditAttribution: MustangGB commentedSo what "needs work" here?
Is it the SQLite/PostgreSQL failures, in which case should/can they be handled in a follow-up issue?
Comment #75
joseph.olstadBumping to 7.61. This didn't make it into 7.60.
Comment #76
joseph.olstadComment #77
joseph.olstadComment #78
MustangGB CreditAttribution: MustangGB commentedComment #79
MustangGB CreditAttribution: MustangGB commentedComment #80
izmeez CreditAttribution: izmeez commentedPatch in #68 applies cleanly to the drupal 7.69 release and appears essential for backup_migrate #3023556: Performed update: backup_migrate_update_7303 is causing SQLSTATE[42000]. It is RTBC despite concerns that it may break functionality of other databases.
Comment #81
MustangGB CreditAttribution: MustangGB commentedComment #82
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThe patch from #68 does not apply anymore, so adding reroll. Just to see if the test failures are still present (there was a lot of changes in PostgreSQL recently).
Comment #83
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedWell, PostgreSQL seems good. Attaching updated patch with fixed typo with undefined
$result
. There is one last test failure in SQLite and that is caused by what @David_Rothstein mentioned earlier:It is interesting, that the same behavior is currently in D8/9. And if we do not have added this recently in D7 (or if it was written as db_add_field() + db_add_index()):
Then the SQLite tests will be green despite of this problem.
I tried to workaround the
array_merge()
problem affecting the update of keys with additional logic which will try to keep the current keys and only add new ones (except primary key, which should be overwritten). What do you think?Comment #84
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented