We received the following report from an optimization consultant:
The drupal_queue module has what could arguably called a bug. The following statement: (N is an integer date, seconds since epoch)
update queue set expired = 0 where expired < N
This should really be:
update queue set expired = 0 where expired != 0 and expired < N
The difference is the first update targets 230,000 rows to update, the 2nd targets about 11. And obviously updating the expired to 0 when its already 0 doesn't make much sense. This was being fired off via cron every 5 mins during the run.
Comment | File | Size | Author |
---|---|---|---|
#21 | system_cron-879076-21.patch | 796 bytes | pillarsdotnet |
#13 | mysql.test | 919 bytes | pillarsdotnet |
#13 | postgres.test | 1.01 KB | pillarsdotnet |
#11 | mysql.test | 945 bytes | pillarsdotnet |
#1 | 879076-1_queue_reset.patch | 729 bytes | alex_b |
Comments
Comment #1
alex_b CreditAttribution: alex_b commentedGreat find. This affects Drupal 7 and Drupal Queue module:
should be:
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedUm, couldn't you say this same thing about any update query? It would get boring reading so many queries like SET city=Boston WHERE city!=Boston AND skill=high. Seems like the MySQL compiler could just do this internally (but maybe it doesn't and thats the whole point).
Comment #3
alex_b CreditAttribution: alex_b commentedmoshe: not quite. The first query updates all records that are smaller than REQUEST_TIME (230000 rows in the example above), while the patched query updates only those that aren't 0 (11 rows in the example above). The potential scale of the queue table makes the difference here.
Comment #4
pwolanin CreditAttribution: pwolanin commented@alex_b - are you sure about that? I think mysql does it already. For example, on a test site w/ 4 nodes:
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe following is a NOP on every SQL implemenation I've used:
Comment #6
pwolanin CreditAttribution: pwolanin commentedUnless we can show some actual performance difference, I think this should be closed.
Comment #7
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedHm? I don't see MySQL doing that, or at least it is not documented.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, doesn't seem documented, but seems to happen anyway on MySQL:
Could we get some testing done on PostgreSQL and SQLite?
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe SQLite Driver does that out of the box (because we manually transform the queries), can anyone confirm that PostgreSQL does that too?
Comment #11
pillarsdotnet CreditAttribution: pillarsdotnet commentedOutput of mysql.test:
Downloading and installing postgres now, even though I think it's pointless...
Comment #12
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, redid the scripts because Postgres doesn't have an "unsigned" datatype, and because the numbers were too small to be meaningful.
mysql.test results:
postgresql.test results:
Well I'll be... I guess it does make a difference...
EDIT: removed line-noise from cut-and-paste above.
PPS: The mysql output should also say 16,536 rows... (65536 / 4).
Comment #13
pillarsdotnet CreditAttribution: pillarsdotnet commentedbah. files didn't attach...
Comment #14
neclimdulPostgres probably is doing the "right" thing from a academic standpoint. Don't see why we wouldn't play along. We really don't want to update where expire = 0 so we might as well not rely on the db to figure that out for us.
Comment #15
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging.
This isn't really a "Bug report", as nothing is broken; it just runs sub-optimally under Postgres. Since no new features are introduced, it isn't a "Feature Request", so I guess it's a "Task".
Comment #16
pillarsdotnet CreditAttribution: pillarsdotnet commentedtag.
Comment #17
pyrollo CreditAttribution: pyrollo commentedFrom a data point of view, it could be. But if the database implements triggers they should be triggered on every row (affected or not). That's how it works on Oracle databases and I guess on PostgreSQL.
This can explain why PostgreSQL performance are so different for the two requests.
Comment #18
neclimdulThere's your academic(actually reasonable real world) explanation. Either way, we're not trying to re-update the old ones but expire new items so I'm for the addition. It makes even more sense to avoid in the event triggers would be run on the old items.
Comment #19
pyrollo CreditAttribution: pyrollo commentedI agree with you : unless you actually want to trigger something you should only update what is needed.
I did not agree with the idea that an update leaving unchanged data is always equivalent to no operation.
Comment #20
Dave ReidAdding tag since this change would need to be backported to http://drupal.org/project/drupal_queue when fixed in core.
Comment #21
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-roll for d8, better title, and tags.
Comment #22
pwolanin CreditAttribution: pwolanin commentedStyle question - do we use '<>' or '!=' for core? I thought the latter.
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe use
<>
, that is the ANSI operator.Comment #24
neclimdulI don't know if its documented anywhere but I'd prefer the ANSI operator as well.
Comment #25
pwolanin CreditAttribution: pwolanin commentedMaybe we need to start a companion page for Drupal 7? This looks a bit stale: http://drupal.org/node/2497
Comment #26
pillarsdotnet CreditAttribution: pillarsdotnet commented@#22, #23, #24, #25 -- Does any of this require a change to the patch in #21? Should it change to "needs work" or "reviewed & tested"?
Comment #27
pillarsdotnet CreditAttribution: pillarsdotnet commented#21: system_cron-879076-21.patch queued for re-testing.
Comment #28
pillarsdotnet CreditAttribution: pillarsdotnet commentedAny reason this shouldn't be RTBC ?
Comment #29
webchickBecause no one else has reviewed it yet. :)
Comment #30
pillarsdotnet CreditAttribution: pillarsdotnet commented#21: system_cron-879076-21.patch queued for re-testing.
Comment #31
pillarsdotnet CreditAttribution: pillarsdotnet commented#21: system_cron-879076-21.patch queued for re-testing.
Comment #32
neclimdulI've been running this in production for a couple weeks with the drupal_queue module without issue(mysql). Being as this isn't adding any odd functionality just a small modification to make ACID compliant DB's faster and the only semi negative reviews have been code standard related and addressed I'll mark this RTBC.
Comment #33
webchickCool, thanks neclimdul!
Committed and pushed to 8.x and 7.x.
Comment #34
Dave ReidThis has been fixed in drupal_queue.module for Drupal 6 as well.
http://drupalcode.org/project/drupal_queue.git/commit/07374f2