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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cafuego’s picture

Version: 6.12 » 7.x-dev
Status: Active » Needs review
FileSize
2.7 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

cafuego’s picture

Status: Needs work » Needs review
FileSize
2.85 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

cafuego’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

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.

sun’s picture

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.

cafuego’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
2.95 KB

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

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

cafuego’s picture

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

sun’s picture

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.

Dries’s picture

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.

cafuego’s picture

Thankyou Dries and sun.

I've found one more - same issue, different table. See #497684: system.install incorrectly assumes more sequential IDs

sun’s picture

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.

andypost’s picture

Simple patch for pgsql install - just change sequince

Berdir’s picture

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

andypost’s picture

FileSize
699 bytes

This is a same fixed patch with comment

andypost’s picture

FileSize
1.04 KB

Another patch - use sequences to insert built-in roles

Berdir’s picture

Well, the last patch reverted what this issue has done, that was the actual change ;)

Damien Tournoud’s picture

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.

Damien Tournoud’s picture

Status: Needs review » Needs work
andypost’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

So this converted to dbtng

Now we need tests.

Damien Tournoud’s picture

Status: Needs review » Needs work
+  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').

andypost’s picture

When we insert primary key sequence is not working

Suppose we need insert only "name" attribute

cafuego’s picture

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.

Damien Tournoud’s picture

@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.

cafuego’s picture

Status: Needs work » Needs review
FileSize
6.84 KB

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.

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PostgreSQL Surge
FileSize
7.02 KB

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.02 KB

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

LOL. That was awesome.

webchick’s picture

Status: Needs work » Needs review

Oh ok. Now #22 is possessed. Sigh.

webchick’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay. Testing bot approves. Committed to HEAD.

axyjo’s picture

Status: Fixed » Needs review
Issue tags: -PostgreSQL Surge
FileSize
710 bytes

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.

axyjo’s picture

Issue tags: +PostgreSQL Surge

Accidentally removed a tag. Sorry.

Damien Tournoud’s picture

Issue tags: +Quick fix
Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Whoopsie. Thanks!

cafuego’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review
FileSize
5.07 KB

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.

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.06 KB

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

deekayen’s picture

Issue tags: +PIFR 2.x blocker

tagging PIFR 2 blocker

deekayen’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I seem to find bugs by simple source code inspection:

+  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.

yonailo’s picture

subscribing

Josh Waihi’s picture

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

cafuego’s picture

Status: Needs work » Needs review
FileSize
6.24 KB

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

andypost’s picture

Looks good for me

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community

Am happy

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)
Issue tags: -PostgreSQL Surge, -Quick fix

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