system.install incorrectly assumes ANON and AUTH RID are always sequential
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | PostgreSQL Surge, Quick fix |
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| bootstrap-sequential-rid-fix.patch | 2.07 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
The same issue exists in Drupal 7, the patch for D7 is attached to this comment.
#2
The last submitted patch failed testing.
#3
Forgot a subst var in the query; redid patch against the latest CVS checkout.
#4
The last submitted patch failed testing.
#5
It 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.
#6
Besides the following change, this looks like a nice improvement:
- db_query("INSERT INTO {filter_format} (name, roles, cache) VALUES ('%s', '%s', %d)", 'Filtered HTML', ',1,2,', 1);+ db_query("INSERT INTO {filter_format} (name, roles, cache) VALUES ('%s', '%s', %d)", 'Filtered HTML', sprintf(",%d,%d,", DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID), 1);
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.
#7
Query rewritten to use the dfined inline and without sprintf() call.
I've also updated the patch for 6.x to include this change.
#8
New versions that adhere to string concatenation coding standards ;-)
#9
The substantial change is:
- db_query("INSERT INTO {role} (name) VALUES ('%s')", 'anonymous user');- db_query("INSERT INTO {role} (name) VALUES ('%s')", 'authenticated user');
+ db_query("INSERT INTO {role} (rid, name) VALUES (%d, '%s')", DRUPAL_ANONYMOUS_RID, 'anonymous user');
+ db_query("INSERT INTO {role} (rid, name) VALUES (%d, '%s')", DRUPAL_AUTHENTICATED_RID, 'authenticated user');
Looks good to me.
#10
Committed to CVS HEAD. Thanks sun and cafuego -- sun for supporting cafuego, cafuego for working on the patch.
Moving version to Drupal 6.
#11
Thankyou Dries and sun.
I've found one more - same issue, different table. See #497684: system.install incorrectly assumes more sequential IDs
#12
I fear that this patch broke Postgres. Sorry for being blind.
Needs some advanced input from sub-system maintainers.
#13
Simple patch for pgsql install - just change sequince
#14
patch doesn't look right, the query is executed twice...
#15
This is a same fixed patch with comment
#16
Another patch - use sequences to insert built-in roles
#17
Well, the last patch reverted what this issue has done, that was the actual change ;)
#18
Those 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.
#19
#20
So this converted to dbtng
Now we need tests.
#21
<?php+ db_insert('test')
+ ->fields(array('rid', 'name'))
+ ->values(array(
+ 'rid' => DRUPAL_ANONYMOUS_RID,
+ 'name' => 'anonymous user',
+ ))
+ ->values(array(
+ 'rid' => DRUPAL_AUTHENTICATED_RID,
+ 'name' => 'authenticated user',
+ ))
+ ->execute();
?>
That should be
db_insert('role').#22
When we insert primary key sequence is not working
Suppose we need insert only "name" attribute
#23
But 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:
$new_rid = db_insert('role')->fields(array('name'))
->values(array(
'name' => 'anonymous user',
));
db_update('role')
->fields(array('rid' => DRUPAL_ANONYMOUS_RID)),
->condition('rid' => $new_rid)
->execute();
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.
#24
@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.
#25
I'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.
#26
Ilike 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.
#27
The last submitted patch failed testing.
#28
Uploading again to try and trigger a faster testing slave. I want to go to bed. ;P
#29
LOL. That was awesome.
#30
Oh ok. Now #22 is possessed. Sigh.
#31
#32
Yay. Testing bot approves. Committed to HEAD.
#33
Okay, 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.
#34
Accidentally removed a tag. Sorry.
#35
#36
#37
Whoopsie. Thanks!
#38
Issue 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.
#39
Tested on PostgreSQL, works a treat. removed the whitespace at the bottom of the function.
#40
tagging PIFR 2 blocker
#41
removing tag cause the PIFR part was already committed and I didn't read the issue history like I should have
#42
I seem to find bugs by simple source code inspection:
<?php
+ if ($rid_anonymous != DRUPAL_ANONYMOUS_RID) {
+ db_query("UPDATE {role} SET rid = %d WHERE rid = %d", DRUPAL_ANONYMOUS_RID, $rid_anonymous);
+ }
+ if ($rid_anonymous != DRUPAL_AUTHENTICATED_RID) {
+ db_query("UPDATE {role} SET rid = %d WHERE rid = %d", DRUPAL_AUTHENTICATED_RID, $rid_authenticated);
+ }
?>
Look, the second $rid_anonymous is not supposed to be $rid_anonymous is it? Let's fix and review this a bit better.
#43
subscribing
#44
... 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?
#45
Updated D6 patch attached. (D7 version was fine all along)
#46
Looks good for me