The-seo-framework: Exclude from archive slow query - reverting to previous version

[kevinlisota]

You introduced the following feature in the latest release, designed to exclude posts from displaying in archives.

https://github.com/sybrew/the-seo-framework/issues/162

There are two major issues with this feature.

First, this introduces another join into our archive queries with the wp_postmeta table that was not present previously. This dramatically slows down archive queries. Prior to this feature, our archive main queries took between 0.1-0.15 sec. Adding this feature slowed down the query time to 0.9-1.0 sec. We have 30,000 posts and 1M rows in our postmeta table.

This is not a performant query and is not production ready on a high traffic site. We had to remove it after 10 mins on the site because it was crippling performance.

Of equal importance, we use lots of secondary WP_Query on our pages. Your modification of pre_get_posts is not checking whether the query is the main query and the result is that it modified all sorts of queries it shouldn't have, both from our theme and from other plugins targeting home or archive pages.

You need to use a check for $query->is_main_query() if you are going to attempt a feature like this on the pre_get_posts hook, otherwise you will stomp on other people's queries in unintended ways.

I'm not certain how you can achieve this feature in a performant way. Adding an unneeded join to all of these queries is a showstopper for us and will never be efficient when combined withe the taxonomy joins already present on archive pages.

[sybrew]

For future reference, the call + SQL flow:

1. https://github.com/WordPress/WordPress/blob/93db53b77d2c688db9f5d55539cad5b28541d13e/wp-includes/class-wp-query.php#L1683
2. https://github.com/sybrew/the-seo-framework/blob/8d14e307672ea0b48ed76aaa95488a48417d087c/inc/classes/init.class.php#L244-L251
3. https://github.com/sybrew/the-seo-framework/blob/8d14e307672ea0b48ed76aaa95488a48417d087c/inc/classes/init.class.php#L629-L711
4. https://github.com/WordPress/WordPress/blob/93db53b77d2c688db9f5d55539cad5b28541d13e/wp-includes/class-wp-meta-query.php#L146
5. https://github.com/WordPress/WordPress/blob/93db53b77d2c688db9f5d55539cad5b28541d13e/wp-includes/class-wp-meta-query.php#L244-L282
6. https://github.com/WordPress/WordPress/blob/93db53b77d2c688db9f5d55539cad5b28541d13e/wp-includes/class-wp-meta-query.php#L582
7.1. https://github.com/WordPress/WordPress/blob/93db53b77d2c688db9f5d55539cad5b28541d13e/wp-includes/class-wp-query.php#L2096 // GROUP BY
7.2. https://github.com/WordPress/WordPress/blob/93db53b77d2c688db9f5d55539cad5b28541d13e/wp-includes/class-wp-query.php#L2146 // JOIN
8. https://dev.mysql.com/doc/refman/5.7/en/internal-temporary-tables.html => issue.

[sybrew]

Three solutions, whereof the first two are temporary:

1. Self: Add a custom filter to stop this madness.
2. Plugin: Add a new option to remove and disable the settings.
3. Plugin: Improve the query.

[sybrew]

Solution 1, your very own adjustment, which will stop the query adjustments from happening:

add_action( 'init', function() {
    $tsf = function_exists( 'the_seo_framework' ) ? the_seo_framework() : null;
    if ( isset( $tsf ) ) {
        remove_action( 'pre_get_posts', array( $tsf, 'adjust_search_filter' ), 9999 );
        remove_action( 'pre_get_posts', array( $tsf, 'adjust_archive_query' ), 9999 );
    }
} );

[sybrew]

~Step 2 will be skipped, this was #154.~

Step 3:

1. Create a new option, that maintains the cache of all sorts. This option works like transients but is self-maintained.
2. On post update, a query will run to fetch all post IDs that should be excluded.
3. Save the results in the option.
4. Add the option results to the post__not_in query, instead of running a GROUP + JOIN clause (which was essentially tripling the query size).

If this doesn't resolve all query-related performance issues, Step 2 will be reconsidered.

[kevinlisota]

For different reasons, posts__not_in can also be slow. First, it will lower the query cache hitrate. If the exclusion list is large, the query will always be slow.

WordPress VIP recommends doing the logic of exclusion in PHP instead:

https://vip.wordpress.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/

[sybrew]

posts__not_in is significantly better than what's being used now.

I looked into whether your suggestion is feasible. Unfortunately, we require to alter this exact loop for it all to work:

<?php if ( have_posts() ) : ?>
    <?php while ( have_posts() ) : the_post(); ?>    
    <!-- do stuff ... -->
    <?php endwhile; ?>
<?php endif; ?>

None of that code can be adjusted JIT without breaking some unwritten laws.

In any case, I'll be sure to test the query cache hit rate. There are no found documents on it having any effect. I want to believe Automattic on their statements, but the keywords "often", "can" and "almost" bother me.

About using main query
This is a risk the user has to take. With the option, you mark the post as useless in any archive listing; whether it's in the main loop or not.
Having it only run in the main loop means that it will stay visible in widgets and whatnot, that can also be seen as a bug.

If a plugin author really needs to ignore 3rd parties for their system to work, they should use suppress_filters.

[kevinlisota]

I have never tested the cache hit rate, but I didn't have to. We used a posts__not_in query on our site that grew very slow as soon as the array of post IDs got past 5-10 IDs. The query execution is simpler, but posts__not_in forces mysql to check each ID one-by-one, which is also very slow as the list grows larger. Make sure you have a sufficiently large posts table to see exactly how bad this can be. Ours is 50k rows.

You are still not understanding is_main_query() and its importance here.

The way you have it structured, you are only checking is_archive(), is_home(). When true, it adds your meta query join via pre_get_posts. This means that ANY secondary WP_Query run when those conditionals are true (any archive or home page), regardless of the post types being queried. There are countless useful reasons to run a secondary WP_Query on a page. We use them all over the place. Maybe you want to display a box with a few posts from custom post types, or some date-related display, or a list of recent posts from some other post type or whatever. All of these are going to have your query logic injected and they shouldn't.

There are even useful reasons to run secondary queries and not loop through them to display.

It is not our job to suppress filters. It is the plugin's job (you) to not insert query modifications indiscriminately, which is exactly what you are doing now.

If the function is supposed to "exclude from archive listing," it should do that and no more.

This feature runs counter to why I moved from Yoast to your plugin. The goal was to keep query bloat down and unobstrusive, but this does exactly the opposite.

[sybrew]

If the function is supposed to "exclude from archive listing," it should do that and no more.

That's the issue, queries that aren't the main query can also be archive listings.

The goal was to keep query bloat down and unobtrusive, but this does exactly the opposite.

You're right. The new feature might be a little too much. You're convincing me to bring Step 2 back to the table.

Please know that I keep all that you say in mind, yet the solution is still a very hard thing to conceive.

[sybrew]

The query execution is simpler, but posts__not_in forces mysql to check each ID one-by-one, which is also very slow as the list grows larger.

💡

That's why the foreach loop is faster. Instead of going over all 50k rows, it just goes over the rows that are being displayed (often not more than 20). That's roughly 2500 times fewer comparisons.

That's great. Now I just need to figure out how to hijack the loop without altering the query.

[nickkeenan]

First of all, I say this as someone who loves and uses this plugin on most of my sites. I'd just like to emphatically +1 kevin lisota's comment here:

It is not our job to suppress filters. It is the plugin's job (you) to not insert query modifications indiscriminately, which is exactly what you are doing now.

This new feature is causing all kinds of issues with secondary queries in other plugins that we use on our sites. For instance, we use Posts2Posts to create and query post relationships with the main post that are then displayed and manipulated outside of the loop. The way P2P works is based on WP_Query and it's taken about 2 hours of reading your plugin code and troubleshooting in the dark to narrow down the issue to the adjust_archive_query functionality. It was not intuitive (and obviously not clearly documented, though I get why that that is as a new feature) what was causing the corruption of our other secondary queries until we reverse engineered this plugin.

I'm going to use your workaround filter above, but I'm concerned about all the query types we use regularly that we're going to have to proactively filter because of this functionality, and afraid of what happens to our clients in production if we miss one.

[sybrew]

Hi @nickkeenan

Thanks for your input! It conforms to our beliefs and the approach we're going to take with this feature.

I'm not really understanding your final notes. About what are you concerned, exactly?
Are you concerned that there might be another method altering the query (in the future)?

[sybrew]

Options and a filter have been added.

Following up:

Option 1:

We adjust the query from a meta query to a post__not_in query.

Option 2:

1. We hook into loop_start, grab $this.
2. ???
3. Done.

Goals:

1. It has to work as before.
2. CPU time overhead < 0.00005s per post.
3. CPU time overhead < 0.001s per query @ 20 posts output, regardless of database size.
4. Memory overhead < 10kB on PHP7+.

Concluding

I'm unsure if we can grab all queries with Option 2. We'll see 😄

I'm going to start with Option 2, it's fastest, most reliable and according to ingenious https://github.com/sybrew/the-seo-framework/issues/167#issuecomment-314178393.
If that fails, we'll see how Options 1 goes.

Option 1 will most likely apply to the Search Query. As we don't want to grab all instances on that one. Depending on the architecture applied, we might also go for Option 2 here.

[sybrew]

post__not_in hasn't been used, yet. Because we require maintaining the excluded IDs.

Continue in #182