Download & Extend

system.install incorrectly assumes ANON and AUTH RID are always sequential

Project:Drupal core
Version:6.x-dev
Component:base system
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed (fixed)
Issue tags:PostgreSQL Surge, Quick fix

Issue Summary

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.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix.patch2.07 KBIdleFailed: Failed to apply patch.View details

Comments

#1

Version:6.12» 7.x-dev
Status:active» needs review

The same issue exists in Drupal 7, the patch for D7 is attached to this comment.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix-D7.patch2.7 KBIdleFailed: Failed to install HEAD.View details

#2

Status:needs review» needs work

The last submitted patch failed testing.

#3

Status:needs work» needs review

Forgot a subst var in the query; redid patch against the latest CVS checkout.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix-2-D7.patch2.85 KBIdleFailed: Failed to install HEAD.View details

#4

Status:needs review» needs work

The last submitted patch failed testing.

#5

Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix-3-D7.patch2.96 KBIdleFailed: Failed to apply patch.View details

#6

Status:needs review» needs work

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

Status:needs work» needs review

Query rewritten to use the dfined inline and without sprintf() call.

I've also updated the patch for 6.x to include this change.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix-4-D7.patch2.95 KBIdleFailed: Failed to apply patch.View details
bootstrap-sequential-rid-fix-2-D6.patch2.13 KBIgnored: Check issue status.NoneNone

#8

New versions that adhere to string concatenation coding standards ;-)

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix-5-D7.patch2.96 KBIdleFailed: Failed to apply patch.View details
bootstrap-sequential-rid-fix-3-D6.patch2.14 KBIgnored: Check issue status.NoneNone

#9

Status:needs review» reviewed & tested by the community

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

Version:7.x-dev» 6.x-dev

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

Version:6.x-dev» 7.x-dev
Priority:normal» critical
Status:reviewed & tested by the community» needs review

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

AttachmentSizeStatusTest resultOperations
495956_pg_install.patch687 bytesIdleFailed: Failed to install HEAD.View details
495956_pg_install.patch687 bytesIdleFailed: Failed to install HEAD.View details

#14

patch doesn't look right, the query is executed twice...

#15

This is a same fixed patch with comment

AttachmentSizeStatusTest resultOperations
495956_pg_install.patch699 bytesIdlePassed: 11826 passes, 0 fails, 0 exceptionsView details

#16

Another patch - use sequences to insert built-in roles

AttachmentSizeStatusTest resultOperations
495956_pg_install.patch1.04 KBIdleFailed: Failed to apply patch.View details

#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

Status:needs review» needs work

#20

Status:needs work» needs review

So this converted to dbtng

Now we need tests.

AttachmentSizeStatusTest resultOperations
495956_pg_install.patch1.17 KBIdleFailed: Failed to install HEAD.View details

#21

Status:needs review» needs work

<?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

Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-rid-fix-6-D7.patch6.84 KBIdleFailed: Failed to apply patch.View details

#26

Status:needs review» reviewed & tested by the community

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.

AttachmentSizeStatusTest resultOperations
495956-bootstrap-sequential-rid-fix-D7.patch7.02 KBIdleFailed: Failed to apply patch.View details

#27

Status:reviewed & tested by the community» needs work

The last submitted patch failed testing.

#28

Status:needs work» reviewed & tested by the community

Uploading again to try and trigger a faster testing slave. I want to go to bed. ;P

AttachmentSizeStatusTest resultOperations
495956-bootstrap-sequential-rid-fix-D7.patch7.02 KBIdleFailed: Failed to apply patch.View details

#29

Status:reviewed & tested by the community» needs work

LOL. That was awesome.

#30

Status:needs work» needs review

Oh ok. Now #22 is possessed. Sigh.

#31

Status:needs review» reviewed & tested by the community

#32

Status:reviewed & tested by the community» fixed

Yay. Testing bot approves. Committed to HEAD.

#33

Status:fixed» needs review

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.

AttachmentSizeStatusTest resultOperations
drupal_cron-495956-33-axyjo.patch710 bytesIdlePassed: 11826 passes, 0 fails, 0 exceptionsView details

#34

Accidentally removed a tag. Sorry.

#35

Issue tags:+Quick fix

#36

Status:needs review» reviewed & tested by the community

#37

Status:reviewed & tested by the community» fixed

Whoopsie. Thanks!

#38

Version:7.x-dev» 6.x-dev
Status:fixed» needs review

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.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-id-fix-D6.patch5.07 KBIgnored: Check issue status.NoneNone

#39

Status:needs review» reviewed & tested by the community

Tested on PostgreSQL, works a treat. removed the whitespace at the bottom of the function.

AttachmentSizeStatusTest resultOperations
bootstrap-sequential-id-fix-D6.patch5.06 KBIgnored: Check issue status.NoneNone

#40

Issue tags:+PIFR 2.x blocker

tagging PIFR 2 blocker

#41

Issue tags:-PIFR 2.x blocker

removing tag cause the PIFR part was already committed and I didn't read the issue history like I should have

#42

Status:reviewed & tested by the community» needs work

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

Status:needs work» needs review

Updated D6 patch attached. (D7 version was fine all along)

AttachmentSizeStatusTest resultOperations
495956-bootstrap-sequential-id-fix-D6.patch6.24 KBIgnored: Check issue status.NoneNone

#46

Looks good for me

#47

Status:needs review» reviewed & tested by the community

Am happy

#48

Status:reviewed & tested by the community» fixed

Committed, thanks.

#49

Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.