When installing Drupal, the system.install file populates the role table with the 'anonymous user');
and 'authenticated user' roles and assumes these will be assigned rid 1 and 2 respectively.
When a MySQL server is part of a multiple master cluster, auto_increment integers are never sequential, when two inserts are performed one after the other; an architectural requirement means each MySQL master server in such a cluster must only assign an auto_increment id that can never clash with one that may be assigned by a different master at the same time.
In practice this means that sequential inserts will produce auto_increment IDs that are n+NUM_MASTERS apart; ie: a setup with three masters would produce 1, 4, 7, 10, 13, etc.
Drupal defines the RID for anon and auth users to be 1 and 2 in the bootstrap.inc file, but does not explicitly use these values when first populating the role table. Nor does it use the defines to then populate the permission and input format tables. Instead it uses the hardcoded numbers 1 and 2.
When for instance using logintoboggan and email validation or a custom module that use DRUPAL_AUTHENTICATED_RID, the assignment of the authenticated user role would fail because the rid in the database may not match the defined value from bootstrap.inc.
The attached patch modifies system.install so that it uses the defined values from bootstrap.inc when initially populating the system tables. This will work fine on multi-master MySQL configurations, as well on single-server set-ups. It also prevents possible future problems by using only the defines, so a change of RID only needs to be done once.
Comment | File | Size | Author |
---|---|---|---|
#45 | 495956-bootstrap-sequential-id-fix-D6.patch | 6.24 KB | cafuego |
#39 | bootstrap-sequential-id-fix-D6.patch | 5.06 KB | Josh Waihi |
#38 | bootstrap-sequential-id-fix-D6.patch | 5.07 KB | cafuego |
#33 | drupal_cron-495956-33-axyjo.patch | 710 bytes | axyjo |
#28 | 495956-bootstrap-sequential-rid-fix-D7.patch | 7.02 KB | webchick |
Comments
Comment #1
cafuego CreditAttribution: cafuego commentedThe same issue exists in Drupal 7, the patch for D7 is attached to this comment.
Comment #3
cafuego CreditAttribution: cafuego commentedForgot a subst var in the query; redid patch against the latest CVS checkout.
Comment #5
cafuego CreditAttribution: cafuego commentedIt looks like I may have stumbled across a potential problem with db_query() in this patch. See #497576: Database layer for more details. Rev 3 includes a work-around which runs correctly on my D7.
Comment #6
sunBesides the following change, this looks like a nice improvement:
In general, we don't use sprintf(). Here, you already know that the values are safe, so you can simply concatenate them into the query.
Comment #7
cafuego CreditAttribution: cafuego commentedQuery rewritten to use the dfined inline and without sprintf() call.
I've also updated the patch for 6.x to include this change.
Comment #8
cafuego CreditAttribution: cafuego commentedNew versions that adhere to string concatenation coding standards ;-)
Comment #9
sunThe substantial change is:
Looks good to me.
Comment #10
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks sun and cafuego -- sun for supporting cafuego, cafuego for working on the patch.
Moving version to Drupal 6.
Comment #11
cafuego CreditAttribution: cafuego commentedThankyou Dries and sun.
I've found one more - same issue, different table. See #497684: system.install incorrectly assumes more sequential IDs
Comment #12
sunI fear that this patch broke Postgres. Sorry for being blind.
Needs some advanced input from sub-system maintainers.
Comment #13
andypostSimple patch for pgsql install - just change sequince
Comment #14
Berdirpatch doesn't look right, the query is executed twice...
Comment #15
andypostThis is a same fixed patch with comment
Comment #16
andypostAnother patch - use sequences to insert built-in roles
Comment #17
BerdirWell, the last patch reverted what this issue has done, that was the actual change ;)
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedThose queries need to be tranformed into proper db_insert(). When they will be, we will be able to deal with this case in PostgreSQL properly.
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #20
andypostSo this converted to dbtng
Now we need tests.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat should be
db_insert('role')
.Comment #22
andypostWhen we insert primary key sequence is not working
Suppose we need insert only "name" attribute
Comment #23
cafuego CreditAttribution: cafuego commentedBut that will re-break MySQL.
Would it be possibly to only insert the role, then retrieve the assigned rid and run an update query to set it to the value of DRUPAL_ANONYMOUS_RID or DRUPAL_AUTHENTICATED_RID? Something like:
That would mean that on systems where the assigned rid is correct (postgres) it would not be changed, whilst on a mysql setup like mine an incorrect one would first be assigned, but then updated to the correct value.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commented@andypost: this is something that needs to be dealt separately in each driver. The scope of this patch is to make that possible, ie. using a proper db_insert() that the PostgreSQL driver will be able to alter correctly.
Comment #25
cafuego CreditAttribution: cafuego commentedI've reworked my patch to use a normal insert an then update the assigned rid as per my comment above. It works OK on MySQL, but could do with a test on PgSQL.
Note, this also contains the patch from #497684: system.install incorrectly assumes more sequential IDs, which addresses the auto-assigned integer problem in the filters table, but should not suffer from the problem on postgres, as described above. That patch also converts some additional queries in the same function to DBTNG.
Comment #26
Josh Waihi CreditAttribution: Josh Waihi commentedIlike it, seems to work nicely, I've added conditional statements around the db_update bits so they only execute when the RIDs don't match up. Also changed the comment - think it makes better sense.
Comment #28
webchickUploading again to try and trigger a faster testing slave. I want to go to bed. ;P
Comment #29
webchickLOL. That was awesome.
Comment #30
webchickOh ok. Now #22 is possessed. Sigh.
Comment #31
webchickComment #32
webchickYay. Testing bot approves. Committed to HEAD.
Comment #33
axyjo CreditAttribution: axyjo commentedOkay, it looks like this caused a regression bug ( #506162: Cron key isn't unserialized). The cron key shouldn't be serialized anymore because variable_set automatically handles that. Here's a patch for it.
Comment #34
axyjo CreditAttribution: axyjo commentedAccidentally removed a tag. Sorry.
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #37
webchickWhoopsie. Thanks!
Comment #38
cafuego CreditAttribution: cafuego commentedIssue reopened for D6.
I've rewritten the patch for this issue on D6, it applies cleanly to 6.13. The patch includes the sanity check, so that the assigned role ids are only changed if they were inserted with incorrect values. This should mean postgresql support will not break with this patch.
The filter assignments also use the assigned ids. I've not modified the variable assignments.
Comment #39
Josh Waihi CreditAttribution: Josh Waihi commentedTested on PostgreSQL, works a treat. removed the whitespace at the bottom of the function.
Comment #40
deekayen CreditAttribution: deekayen commentedtagging PIFR 2 blocker
Comment #41
deekayen CreditAttribution: deekayen commentedremoving tag cause the PIFR part was already committed and I didn't read the issue history like I should have
Comment #42
Gábor HojtsyI seem to find bugs by simple source code inspection:
Look, the second $rid_anonymous is not supposed to be $rid_anonymous is it? Let's fix and review this a bit better.
Comment #43
yonailo CreditAttribution: yonailo commentedsubscribing
Comment #44
Josh Waihi CreditAttribution: Josh Waihi commented... Sorry :S
I'm not able to re-write the patch at the moment because I'm away on holiday, away from machines I can use to write patches.
Anyone able?
Comment #45
cafuego CreditAttribution: cafuego commentedUpdated D6 patch attached. (D7 version was fine all along)
Comment #46
andypostLooks good for me
Comment #47
Josh Waihi CreditAttribution: Josh Waihi commentedAm happy
Comment #48
Gábor HojtsyCommitted, thanks.