I noticed that the services_user table has no key, best practices suggest that it has a primary key

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jrglasgow created an issue. See original summary.

jrglasgow’s picture

Status: Active » Needs review
FileSize
607 bytes

here is a patch that will make the uid field a primary key

marcingy’s picture

Status: Needs review » Needs work

Nice catch

+
+
+/**
+ * Add primary key to the workbench_notification_messages table
+ */
+function services_update_7403() {
+  db_add_primary_key('services_user', array('uid'));
+}
\ No newline at end of file

Couple of small things we need a newline at end of file and . after the doxygen comment.

tyler.frankenstein’s picture

Aside from the missing period and newline, this seems legit. Except I got this error/warning on a simple dummy D7 localhost site:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '15' for key 'PRIMARY'

I can see this in the services_user table, so it seems like this update function should purge all rows except the most recent for each user id. I can't speak to what this table is actually used for, I'll defer to Kyle and @marcingy on that one.

select * from services_user;
+-----+------------+------------+
| uid | created    | changed    |
+-----+------------+------------+
|  15 | 1453484346 | 1458088391 |
|  15 | 1453484349 | 1458088391 |
|  15 | 1453484667 | 1458088391 |
|  15 | 1453484710 | 1458088391 |
|  15 | 1453484713 | 1458088391 |
|  15 | 1456881915 | 1458088391 |
|  15 | 1456881919 | 1458088391 |
|  15 | 1456881992 | 1458088391 |
|  15 | 1456881994 | 1458088391 |
|  15 | 1458088385 | 1458088391 |
|  15 | 1458088389 | 1458088391 |
|  17 | 1459979983 | 1459979983 |
+-----+------------+------------+
hanoii’s picture

Priority: Normal » Critical
FileSize
1.37 KB

There's a pretty huge bug in the code as it is, see _user_resource_update_services_user

    db_update('services_user')
      ->fields(array(
          'uid' => $uid,
          'changed' => $time,
        )
      )
      ->execute();

The update is updating EVERY ROW, not that the row specifically to the uid.

This is interesting, I was tracing a performance error on a production site with a huge userbase and api exposure and I found out this query was taking a lot, and it was because on each update, around 500k were executed, and that happens pretty often.

Attached is a patch that fixes this, adds the primary key as per #2, corrected minor coding standards and also truncate the table on the update. The data at this point on any site should be meaningless as every site should have the uid updated the the latest operation, so that's why I am truncating it.

I also searched and this table is really not being used anywhere, so I rather wonder if we should simply remove it instead.

It was added on http://cgit.drupalcode.org/services/commit/?h=7.x-3.x&id=809aafac6928e89..., maybe there was a reason that then was forgotten?

hanoii’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

Proper truncate.

hanoii’s picture

Title: services_user table has no key » services_user table has no key and is updating every row
kylebrowning’s picture

Isn't it used in _user_resource_create and _user_resource_update?

hanoii’s picture

@kylebrowning, yes, I didn't express myself properly. It is used there, only to store/update on the services_user table, but then, that data is not used anywhere else. I see no point in storing something in a database that's not being used later on.

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community

Its a record to store that services updated the user.

Its incase we ever have to do any kind of security update around users again because of what happened in that actually patch.

Im not okay with removing it, but am happy with this patch.

tyler.frankenstein’s picture

@kylebrowning, are you OK with the hook_update_N() truncating the table?

kylebrowning’s picture

Status: Reviewed & tested by the community » Needs work

Crap how did I not see that.

No we shouldn't truncate, whats the point of truncating? We should at least be able to see if the user was touched by services, if the dates wrong, oh well.

hanoii’s picture

The truncate is deliberate, see what I explained on #5, there's really no much use of this table as it is. The db_update was affecting every row in the table, which means that the uid of EVERY ROW matches the uid of the last user services updated, with its changed overwritten to the last changed timestamp. The only use case where this table could contain reliable data is if there were only inserts. I really believe truncate it it's the most sensible solution at this point.

See even @tyler.frankenstein resultset on #4 he has a bunch of 15s with the same changed timestamp (likely last update before 17, well, by looking at the created of 17, it is actually after the last updated timestamp of 15) and then a 17, likely an insert. Next time service updates the user, say updating 15 again, we will have all 15, with the same changed timestamp on all rows.

EDIT: Worth mentioning that all of those 15 are not really updates on 15, but rather inserts where the uid was lost.

Do you have other ideas? Do you think something else worth doing?

hanoii’s picture

Status: Needs work » Needs review
hanoii’s picture

Sorry, bumping this one up, it should be kind of a serious things for sites doing heavy user operations and would be simple to sort it out.

I know the truncate smells, however, I really don't see any other way.

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community

tyler.frankenstein’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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