Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I noticed that the services_user table has no key, best practices suggest that it has a primary key
Comment | File | Size | Author |
---|---|---|---|
#6 | services_services-user-table-fix-2701483-6.patch | 1.38 KB | hanoii |
Comments
Comment #2
jrglasgow CreditAttribution: jrglasgow commentedhere is a patch that will make the uid field a primary key
Comment #3
marcingy CreditAttribution: marcingy commentedNice catch
Couple of small things we need a newline at end of file and . after the doxygen comment.
Comment #4
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedAside 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.
Comment #5
hanoiiThere's a pretty huge bug in the code as it is, see
_user_resource_update_services_user
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?
Comment #6
hanoiiProper truncate.
Comment #7
hanoiiComment #8
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedIsn't it used in _user_resource_create and _user_resource_update?
Comment #9
hanoii@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.
Comment #10
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedIts 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.
Comment #11
tyler.frankenstein CreditAttribution: tyler.frankenstein commented@kylebrowning, are you OK with the hook_update_N() truncating the table?
Comment #12
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedCrap 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.
Comment #13
hanoiiThe 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?
Comment #14
hanoiiComment #15
hanoiiSorry, 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.
Comment #16
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedComment #18
tyler.frankenstein CreditAttribution: tyler.frankenstein commented