Discovered as a part of investigating using Drupal with millions of nodes:

The SQL query in ping_cron() does a full table scan of the {node} table

CommentFileSizeAuthor
#4 336358-D6.patch797 bytesWesley Tanaka
#7 336358-D6.patch1.05 KBWesley Tanaka
#6 336358-remove-table-scan-from-ping_cron-D6.patch1.05 KBbrianV
#1 336358-D6.patch796 bytesWesley Tanaka

Comments

Wesley Tanaka’s picture

Wesley Tanaka commented
Status: Active » Needs review
StatusFileSize
new 336358-D6.patch796 bytes
brianV’s picture

brianV commented
Status: Needs review » Reviewed & tested by the community

Tested, and looks good.

Ping module is not in core in D7, so this should go straight into D6.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Is this better because changed has an index, but created does not? (I did not look that up). Also, can we have the comment capitalized and have a dot at the end?

Wesley Tanaka’s picture

Wesley Tanaka commented
StatusFileSize
new 336358-D6.patch797 bytes

Both changed and created have indexes, but whatever versions of MySQL that I have tried do not make this optimization of splitting the OR clause into two separate queries to avoid the full table scan.

The order of the queries is just a heuristic to try to get the second query to be short-circuited out most of the time.

Patch file edited directly to capitalize comment and add a period

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Why are we not using our %d placeholder in the query then?

brianV’s picture

brianV commented
Status: Needs work » Needs review
StatusFileSize
new 336358-remove-table-scan-from-ping_cron-D6.patch1.05 KB

@Gabor,

That was an oversight when I initially RTBC'd this. I have fixed that in this version.

Wesley Tanaka’s picture

Wesley Tanaka commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 336358-D6.patch1.05 KB

I did not originally use %d placeholders because the original code did not use %d placeholders.

Given that we're cleaning that up, I'd also suggest switching the double quoted strings to single quoted ones (single quotes being faster). I have edited the patch to do this and am attaching it here.

Otherwise this %d version of the patch looks good to me.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.6 » 6.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks, committed to Drupal 6.

Status: Fixed » Closed (fixed)

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