WP_Comment_Query loses sql_clauses with Object Cache

[Rarst]

Once I updated my live site to WP 4.6 my error log started filling up with following:
> WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE AND comment_parent IN (266250,266252,266255,266258,266259,266262,266264,2' at line 1 for query SELECT wp_comments.comment_ID, wp_comments.comment_parent WHERE AND comment_parent IN (266250,266252,266255,266258,266259,266262,266264,266267,266269) ORDER BY comment_date_gmt ASC, comment_ID ASC made by require('wp-blog-header.php'), require_once('wp-includes/template-loader.php'), include('/themes/twentysixteen/single.php'), comments_template, WP_Comment_Query->__construct, WP_Comment_Query->query, WP_Comment_Query->get_comments, WP_Comment_Query->fill_descendants, QM_DB->query

Note the highly invalid query, missing parts like FROM altogether.

The immediate cause seems to be sql_clauses property is not meaningfully filled at that point.

Issue goes away if I disable Object Cache (Siteground's Memcached). If I flush cache I see clauses filling up correctly first time, then going blank again on reload.

I would guess this is related to caching changes that shipped, but I cannot reproduce locally with different OC (APCu) or figure out at which point it goes wrong.

[boonebgorges]

Possibly related: #36487, #37184

@Rarst Thanks very much for the ticket. It appears that this is the same bug as #36487, but arises when the cache is primed for only a subset of all the matched comments. I'll run some tests.

In case it's relevant, what is QM_DB? I assume this is a db.php drop-in?

[Rarst]

Replying to boonebgorges:

In case it's relevant, what is QM_DB? I assume this is a db.php drop-in?

That's from Query Monitor plugin, I think it only does drop-in for the purpose of more thorough logging.

The query goes wrong before it's executed against DB in this case, so shouldn't be relevant I think.

[boonebgorges]

Thanks, @Rarst.

I looked into this a bit more, and it does seem to be related to a partially primed cache. I'm uncertain how this happens in the wild, since all caches are linked to the same 'last_changed' incrementor - meaning that they should all stand or fall together. Clearly, though, there are cases where they can be out of sync, so the query in question should be more resilient.

In 37696.diff​, I discarded the technique that relied on the existing SQL clauses, in favor of a fresh call to get_comments(). There's also a test there that contrives a situation that demonstrates the failure.

@Rarst Are you able to test this patch in an environment where you're experiencing the bug?

[Rarst]

I have applied patch on live site and it seems to resolve the issue, I no longer see errors logged on a load of page with comments.

I had cache-related issues on this site before, for a while every core update was locking me out of admin due to some mismatch in database version between cache and DB.

If you want more insight into that I would recommend to get in touch with SiteGround folks (I have reported issue to them as well, with link to this ticket).

[boonebgorges]

Replying to Rarst:

I have applied patch on live site and it seems to resolve the issue, I no longer see errors logged on a load of page with comments.

I had cache-related issues on this site before, for a while every core update was locking me out of admin due to some mismatch in database version between cache and DB.

If you want more insight into that I would recommend to get in touch with SiteGround folks (I have reported issue to them as well, with link to this ticket).

This is really helpful - thanks, @Rarst.

Can I get a sanity check from someone who understands the arcana contained in fill_descendants()? Ping @wonderboymusic, @rachelbaker

[Rarst]

SiteGround support was able to reproduce on my account with clean from scratch WP 4.6 install. Doesn’t sound like they got any idea why their caching setup doesn't like it though. And since you got patch posted they closed my ticket. :)

Let me know if I can look into any info necessary about caching kinks. I have a hunch that if this isn't resolved it will keep coming up in different contexts.

[danielkanchev]

Hey, guys :) Daniel with SiteGround here - if you need a website to test with please ping me - I'll be glad to assist.

[dd32]

Replying to boonebgorges:

I looked into this a bit more, and it does seem to be related to a partially primed cache. I'm uncertain how this happens in the wild, since all caches are linked to the same 'last_changed' incrementor - meaning that they should all stand or fall together. Clearly, though, there are cases where they can be out of sync, so the query in question should be more resilient.

Caches evict data at different rates, the cached content may be evicted before the expiry, only half the caches related to a bit of code might get evicted at that point in time and others will live on for another year. Caches evict data based on key size, hit rate, usage, how full the underlying bucket it's in is, and seemingly sometimes rand(0,1).
In other words; No code should expect a cached value to exist, even if another piece of data stored at the same time still exists there, "It doesn't exist unless it's in my hands" (or variable as it may be).

[boonebgorges]

Replying to dd32:

Caches evict data at different rates, the cached content may be evicted before the expiry, only half the caches related to a bit of code might get evicted at that point in time and others will live on for another year. Caches evict data based on key size, hit rate, usage, how full the underlying bucket it's in is, and seemingly sometimes rand(0,1).
In other words; No code should expect a cached value to exist, even if another piece of data stored at the same time still exists there, "It doesn't exist unless it's in my hands" (or variable as it may be).

Thanks @dd32. This confirms the strategy in 37696.diff​.

[Akeif]

Replying to danielkanchev:

Hey, guys :) Daniel with SiteGround here - if you need a website to test with please ping me - I'll be glad to assist.

Hey @danielkanchev I'd take that offer if you're still around. Let me know how we need to proceed.

[danielkanchev]

@Akeif send me a PM on Slack (@daniel_kanchev) and I'll create an account for you that you may use for testing.

[Akeif]

First: Thanks to @danielkanchev for setting a test account.
Second: I was able to reproduce the problem and apply the patch, which fix the issue.

Watch this screen cast if to see the error and patch in action: ​https://www.youtube.com/watch?v=SjLC_O-2tfo

I also tested upgrading, downgrading WordPress but haven't seen any problem there.

I'm removing the need-testing keyword since @Rarst and myself were able to confirm the fix.

Here's the SQL error output from Query Monitor plugin (and I'm attaching a screenshot).

SELECT wpbj_comments.comment_ID, wpbj_comments.comment_parent
WHERE
AND comment_parent IN (1,2)
ORDER BY comment_date_gmt ASC, comment_ID ASC	WP_Comment_Query->fill_descendants()
 wp-includes/class-wp-comment-query.php:999
WP_Comment_Query->get_comments()
 wp-includes/class-wp-comment-query.php:471
WP_Comment_Query->query()
 wp-includes/class-wp-comment-query.php:357
WP_Comment_Query->__construct()
 wp-includes/class-wp-comment-query.php:311
comments_template()
 wp-includes/comment-template.php:1407	Core 	You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE AND comment_parent IN (1,2) ORDER BY comment_date_gmt ASC, comment_ID AS' at line 1

[Akeif]

Query Monitor plugin - Error report

[Rarst]

Fix for specific issue aside I would be very interested why it breaks on SiteGround specifically. Is it something they do with caching? Is it some caching edge case in core?

As I mentioned I had caching issues before, but I as far as I can see their setup is pretty typical. Yet some issues come up there, but not elsewhere.

[boonebgorges]

Replying to Rarst:

Fix for specific issue aside I would be very interested why it breaks on SiteGround specifically. Is it something they do with caching? Is it some caching edge case in core?

As I mentioned I had caching issues before, but I as far as I can see their setup is pretty typical. Yet some issues come up there, but not elsewhere.

It's possible that it's specific the Memcached config in use at SiteGroup. Memcached sorts data into buckets based on the size of the value being stored, which could result in different rules for invalidation, depending on how Memcached is set up. As @dd32 notes, our caching should be resilient against this kind of thing, so as far as I know SG is not doing anything wrong, but I agree it would be nice to have more details on why it's so easily reproducible there.

[swissspidy]

Any update on this? The patch seems to resolve the issue and it should go into trunk rather sooner than later to be considered for 4.6.1.

[boonebgorges]

Replying to swissspidy:

Any update on this? The patch seems to resolve the issue and it should go into trunk rather sooner than later to be considered for 4.6.1.

The pre-commit review I was hoping for appears not to be forthcoming. I've done a second review myself, and will commit to trunk to get some testing before branch merge.

[boonebgorges]

In 38446:

Comments: Don't do direct SQL query when fetching decendants.

The SQL query was built using the clauses compiled when querying for
top-level comments. But in cases where the top-level comment query
results are already in the cache, the SQL clauses are not built, and
so are unavailable for fill_descendants(). Instead, we call
get_comments(), using modified versions of the parameters passed
to the main WP_Comment_Query class.

Props Akeif, Rarst for testing.
Fixes #37696.

[boonebgorges]

Reopening for 4.6.1.

[jeremyfelt]

I stared at the code around this for a while and the approach looks correct/sane. +1

To log the obvious - The previous manual SQL statement uses SELECT wp_comments.comment_ID, wp_comments.comment_parent. The current get_comments() approach generates SELECT wp_comments.comment_ID. This is okay because the comment parent is instead available in the retrieved comment object.

[jeremyfelt]

In 38497:

Comments: Don't do direct SQL query when fetching decendants.

The SQL query was built using the clauses compiled when querying for
top-level comments. But in cases where the top-level comment query
results are already in the cache, the SQL clauses are not built, and
so are unavailable for fill_descendants(). Instead, we call
get_comments(), using modified versions of the parameters passed
to the main WP_Comment_Query class.

Merge of [38446] to the 4.6 branch.

Props boonebgorges, Akeif, Rarst for testing.
Fixes #37696.

[remedy17]

Hi folks, my site is hosted at SiteGround, have the same issue with a missing strings in SQL query.

.....at line 1]
SELECT wp_comments.comment_ID, wp_comments.comment_parent WHERE AND comment_parent IN (116) ORDER BY comment_date_gmt ASC,....

Is the patch something that I can borrow from you guys at least until WP 4.6.1 or whenever it makes it into core?

[danielkanchev]

Hey @remedy17 Daniel with SiteGround here :) The patch that needs to be applied is:

https://core.trac.wordpress.org/attachment/ticket/37696/37696.diff

If you cannot apply it open a support ticket and our support team will apply the patch for you.

[swissspidy]

@remedy17 There's also 4.6.1 RC1 you could install on your site: ​https://wordpress.org/wordpress-4.6.1-RC1.zip

[jeremyfelt]

In 38537:

Comments: Revert [38497]

Restore direct SQL query when fetching descendants.

See #37696, #37966.

[swissspidy]

#37966 was marked as a duplicate.

[boonebgorges]

Reopening this ticket because of #37966, which shows that [38497] introduced a bug related to threaded comments on pages other than 1. I'm pretty sure that this is because the argument array for the descendant query isn't resetting the 'number' and/or 'offset' parameters from the original query args.

Unfortunately, the bug was discovered just before the scheduled release of 4.6.1. In order not to hold up 4.6.1, we're moving this ticket out of the milestone.

[boonebgorges]

The problem reported in #37966 is fixed by 37696.2.diff​. Here's the logic, which I hope is clear enough to get a review :)

• To minimize SQL queries, comment trees are fetched one level at a time. So if A and B are top-level comments, with children A1, A2, B1, a query that fetches A and B will do two queries. The first will grab A and B (parent=0) and the second will fetch A1, A2, B1 (parent IN (A,B)). That's what fill_descendants() is for.
• The descendant query should be similar to the parent query. For example, if you get_comments( 'exclude=1,2,3' ), you expect 1, 2, and 3 to be excluded from top-level comments as well as descendants. But the descendant query cannot match exactly, because it needs to override the parent__in parameter; and parent__in should override other hierarchy-related parameters passed to WP_Comment_Query.
• Prior to [38497], the descendant query was being built by parsing and modifying the SQL string for the parent query. This broke, because the SQL string is not available in all cases.
• [38497] changed the strategy so that the descendant query calls get_comments() instead. The parameters passed to get_comments() are those passed to WP_Comment_Query, with some necessary modifications to ensure that we're fetching only the hierarchy level we intend to be fetching.
• What was missed in [38497], and what was reported in #37966, is that the 'offset' and 'number' parameters are not being reset for the descendant query. As such, if you passed offset=1&number=1 to WP_Comment_Query, the descendant query would *also* only fetch a single comment, with an offset of 1. This is incorrect: the descendant query should always have an offset of 0 and no limits on the number of results returned. That's what 37696.2.diff​ adds.

[rachelbaker]

@boonebgorges I will try to get to this over the weekend.

[boonebgorges]

I've done yet another self-review here, and I think the logic is good, so let's go with it.

[boonebgorges]

In 39274:

Comments: Query used to fill comment descendants should reset 'offset' and 'number' params.

Descendant queries should not inherit the 'offset' and 'number'
parameters of the parent query, or descendants will be missed.

Previously: [38497].

See #37696.

[boonebgorges]

I'm going to leave this ticket open on the assumption that there may be a 4.6.2. If there is, both [38497] and [39274] must be ported to the 4.6 branch. If there is no 4.6.2 before 4.7 is released, then this ticket can be closed as fixed.