Do not set expired=0 where expired is already 0.

Great find. This affects Drupal 7 and Drupal Queue module:

// in system_cron():
db_update('queue')
  ->fields(array(
    'expire' => 0,
  ))
  ->condition('expire', REQUEST_TIME, '<')
  ->execute();

should be:

// in system_cron():
db_update('queue')
  ->fields(array(
    'expire' => 0,
  ))
  ->condition('expire', 0, '<>')
  ->condition('expire', REQUEST_TIME, '<')
  ->execute();

Um, 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).

moshe: 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.

@alex_b - are you sure about that? I think mysql does it already. For example, on a test site w/ 4 nodes:

mysql> update node set status = 1;
Query OK, 0 rows affected (0.00 sec)
Rows matched: 4  Changed: 0  Warnings: 0

The following is a NOP on every SQL implemenation I've used:

update foo set bar = baz where bar = baz

Unless we can show some actual performance difference, I think this should be closed.

Hm? I don't see MySQL doing that, or at least it is not documented.

Ok, doesn't seem documented, but seems to happen anyway on MySQL:

mysql> UPDATE comment SET status = 1;
Query OK, 12143 rows affected (0.58 sec)
Rows matched: 12143  Changed: 12143  Warnings: 0

mysql> UPDATE comment SET status = 1;
Query OK, 0 rows affected (0.04 sec)
Rows matched: 12143  Changed: 0  Warnings: 0

Could we get some testing done on PostgreSQL and SQLite?

The SQLite Driver does that out of the box (because we manually transform the queries), can anyone confirm that PostgreSQL does that too?

Output of mysql.test:

Creating test database and seeding with 1024 rows of random data
Dumping test database for repeatable results
Testing 100 iterations of query: "update queue set expire = 0 where expire < 2147483648"
Run 1: real 2.61 user 0.67 sys 0.34
Run 2: real 1.24 user 0.50 sys 0.21
Run 3: real 1.19 user 0.43 sys 0.21
Run 4: real 1.25 user 0.49 sys 0.23
Run 5: real 1.27 user 0.48 sys 0.24
Testing 100 iterations of query: "update queue set expire = 0 where expire <> 0 AND expire < 2147483648"
Run 1: real 1.18 user 0.45 sys 0.21
Run 2: real 1.23 user 0.47 sys 0.24
Run 3: real 1.26 user 0.48 sys 0.23
Run 4: real 1.89 user 0.61 sys 0.30
Run 5: real 1.47 user 0.58 sys 0.25

Downloading and installing postgres now, even though I think it's pointless...

Okay, redid the scripts because Postgres doesn't have an "unsigned" datatype, and because the numbers were too small to be meaningful.

mysql.test results:

Creating test database and seeding with 65536 rows of random data
Dumping test database for repeatable results
Testing 100 iterations of query: "update queue set expire = 0 where expire < 1"
Run 1: real 1.47 user 0.53 sys 0.26
Run 2: real 1.35 user 0.49 sys 0.22
Run 3: real 1.16 user 0.42 sys 0.20
Run 4: real 1.24 user 0.47 sys 0.19
Run 5: real 1.46 user 0.52 sys 0.26
Testing 100 iterations of query: "update queue set expire = 0 where expire <> 0 AND expire < 1"
Run 1: real 1.25 user 0.41 sys 0.20
Run 2: real 1.32 user 0.50 sys 0.27
Run 3: real 1.64 user 0.51 sys 0.26
Run 4: real 1.26 user 0.47 sys 0.21
Run 5: real 1.25 user 0.51 sys 0.21

postgresql.test results:

Creating test database and seeding with 16,536 rows of random data
DROP DATABASE
CREATE DATABASE
NOTICE:  CREATE TABLE will create implicit sequence "queue_qid_seq" for serial column "queue.qid"
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "queue_pkey" for table "queue"
CREATE INDEX
Dumping test database for repeatable results
Testing 100 iterations of query: "update queue set expire = 0 where expire < 1"
Run 1: real 179.28 user 6.00 sys 1.04
Run 2: real 177.24 user 6.26 sys 1.09
Run 3: real 177.75 user 6.34 sys 1.14
Run 4: real 177.29 user 6.23 sys 1.12
Run 5: real 175.35 user 6.33 sys 1.14
Testing 100 iterations of query: "update queue set expire = 0 where expire <> 0 AND expire < 1"
Run 1: real 8.55 user 3.42 sys 0.71
Run 2: real 8.73 user 3.37 sys 0.68
Run 3: real 9.21 user 3.37 sys 0.70
Run 4: real 8.37 user 3.35 sys 0.70
Run 5: real 8.41 user 3.40 sys 0.67

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

bah. files didn't attach...

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

Tagging.

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

tag.

The following is a NOP on every SQL implemenation I've used

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

There'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.

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

Adding tag since this change would need to be backported to http://drupal.org/project/drupal_queue when fixed in core.

Re-roll for d8, better title, and tags.

Style question - do we use '<>' or '!=' for core? I thought the latter.

We use <>, that is the ANSI operator.

I don't know if its documented anywhere but I'd prefer the ANSI operator as well.

Maybe we need to start a companion page for Drupal 7? This looks a bit stale: http://drupal.org/node/2497

@#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"?

#21: system_cron-879076-21.patch queued for re-testing.

Any reason this shouldn't be RTBC ?

Because no one else has reviewed it yet. :)

#21: system_cron-879076-21.patch queued for re-testing.

#21: system_cron-879076-21.patch queued for re-testing.

I'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.

Cool, thanks neclimdul!

Committed and pushed to 8.x and 7.x.

This has been fixed in drupal_queue.module for Drupal 6 as well.
http://drupalcode.org/project/drupal_queue.git/commit/07374f2