Lazy load post meta

[spacedmonkey]

Introduced in [36566], there is was a API introduced to lazily load metadata using the class WP_Metadata_Lazyloader. Currently this is only used comment and term meta data. However, this could be expanded to support over meta types like posts.

WP_Query is now run multiple lines per page load. Meaning that post meta is primed even when it is not needed. Using the WP_Metadata_Lazyloader class, post meta could be loaded on demand and save database queries and object cache lookups.

[spacedmonkey]

Related: #57498

Trac ticket: https://core.trac.wordpress.org/ticket/57496

[spacedmonkey]

@pbearne My patch has been refreshed. This patch scales back the PR, so that lazy loading is opt-in. I have implemented it in a number of places that make sense to me. There maybe more.

This PR needs some tests, but I think it is nearly there.

[spacedmonkey]

I would love the feedback from @peterwilsoncc and @mukesh27 on this one. The PR needs tests, but I would love a review of the approach.

[peterwilsoncc]

@spacedmonkey For term meta, WP_Query accepts an argument to indicate it should be lazy loaded. The proposed PR requires developers to manually call wp_lazyload_post_meta() after making a query that does not prime the post meta.

I think it would be better to add an argument to lazy load post meta within WP_Query so developers can pass the argument and it just works. The new argument would need to affect how update_post_term_cache works, similar to the lazy_load_term_meta argument.

I think that would reduce the chance of developers making mistakes and accidentally increasing the number of database queries when their intent is to limit the post meta query to be made only as required.

[spacedmonkey]

@peterwilsoncc Good call out. I have updated PR with your feedback.

I am much happier with the shape of the PR. Once you are happy, I can add some tests.

@joemcgill @mukeshpanchal27 mind taking a look at this PR?

[flixos90]

It looks like the PR ​https://github.com/WordPress/wordpress-develop/pull/4216 is in need for follow up reviews. Do you think you can take another look @joemcgill?

[joemcgill]

@spacedmonkey this one has some outstanding feedback to address if this is going to land in 6.8. Curious if you'll be able to follow-up before Beta 1. I'm going to reassign this one to you in the meantime for visibility.

@peterwilsoncc I mocked up an idea that I had to track the already loaded meta ids as part of the metadata lazy loading class. ​https://github.com/WordPress/wordpress-develop/pull/8399. This would save lot of lookup to cache values. I see this as a general improve to this class and not linked to this change, but it can be bundled, if you see it a blocker

​https://github.com/WordPress/wordpress-develop/pull/8399

@spacedmonkey I don't think it's a blocker, what is a blocker is failing to process the queue for post types when a databse query will be made. The whole point of lazy loaded meta data is to process the queue alongside another query, the custom code for post meta defeats the purpose of this code.

This test, which is currently failing, demonstrates the issue:

/**
 * @ticket 57496
 */
public function test_post_meta_database_queries_process_the_queue() {
        $lazy_loaded_post_ids = self::$post_ids;
        // Remove one post from the queue.
        $unqueued_post_id = array_shift( $lazy_loaded_post_ids );

        new WP_Query(
                array(
                        'post_type'           => 'wptests_pt',
                        'post__in'            => $lazy_loaded_post_ids,
                        'lazy_load_post_meta' => true,
                )
        );

        // Post meta cache should be empty.
        $cache = wp_cache_get_multiple( self::$post_ids, 'post_meta' );
        $cache = array_filter( $cache );
        $this->assertEmpty( $cache, 'Post meta cache is expected to be empty' );

        // Getting post meta for unqueued post should trigger a database query.
        $queries_before = get_num_queries();
        get_post_meta( $unqueued_post_id );
        $queries_after = get_num_queries();
        $this->assertSame( 1, $queries_after - $queries_before, 'Expected a database query getting unqueued post meta' );

        // Ensure getting post meta for queued posts does not trigger a database query.
        $queries_before = get_num_queries();
        array_map( 'get_post_meta', self::$post_ids );
        $queries_after = get_num_queries();
        $this->assertSame( 0, $queries_after - $queries_before, 'Getting lazy loaded post meta is not expected to trigger a database query' );

        // Ensure all the post meta is now in the cache.
        $cache = wp_cache_get_multiple( self::$post_ids, 'post_meta' );
        $cache = array_filter( $cache );
        $this->assertCount( count( self::$post_ids ), $cache, 'Expected all post meta to be in the cache' );
}

Removing the custom code for post meta allows the test to pass.

I don't think it's a blocker, what is a blocker is failing to process the queue for post types when a databse query will be made. The whole point of lazy loaded meta data is to process the queue alongside another query, the custom code for post meta defeats the purpose of this code.

This test, which is currently failing, demonstrates the issue:

@peterwilsoncc
This PR is not just about reducing database queries by bundling post meta in the queue when post meta is loaded. Let me clarify.

For blog (site), term, and comment meta, WordPress core rarely, if ever, makes use of this data. These types of meta are primarily designed for developers to extend functionality. Because of this, it would be inefficient to load them on every page request, which is why they are lazily loaded.

Post meta, however, is different. It is widely used throughout WordPress core across all post types. Unlike blog, term, or comment meta functions, get_post_meta is called frequently—sometimes over 100 times per page load. As a result, the post meta queue would be constantly cleared, making lazy loading ineffective.

This PR does not aim to bundle post meta queries together. Instead, it creates a queue of post meta IDs that might be needed in a request. Here are two examples:

1. Post meta for parent posts of posts within the loop.
2. Post meta for posts/pages that are referenced within other posts.

The first time an ID in the queue is requested—such as the first menu item linking to a page—all associated meta will be loaded at once. This allows us to disable post meta priming in areas like menus, where the post meta for linked pages is unlikely to be needed. See #60199.

By design, your test would fail, as it is not designed to do what you are asking.

This PR does not aim to bundle post meta queries together. Instead, it creates a queue of post meta IDs that might be needed in a request. Here are two examples: 1. Post meta for parent posts of posts within the loop. 2. Post meta for posts/pages that are referenced within other posts.

The first time an ID in the queue is requested—such as the first menu item linking to a page—all associated meta will be loaded at once. This allows us to disable post meta priming in areas like menus, where the post meta for linked pages is unlikely to be needed. See #60199.

I think what you are describing is the the purpose of the existing update_post_meta_cache option in WP_Query.

The design of lazy loading post meta IS to clear the queue when post meta is requested. Having it behave differently for different types of meta data is simply confusing, and won't result in fewer database queries.

The concern in Core-60199 is a valid concern but the code as written doesn't solve that. Instead of querying post meta in a single query, it increases the chance that post meta will be loaded for each post as a single query.

If loading post meta by single queries is better under some circumstances, then the PR should be targettng that rather than using the existing API.

Re-reading the current discussion and if I'm understanding correctly, the only question that needs to be resolved is whether queued post meta should be loaded only when another queued post meta is called, rather than clearing the queue when any uncached post meta is loaded.

The current approach seems to only load queued meta when another queued meta ID is requested.

I think @peterwilsoncc's argument in ​this comment is valid. To paraphrase:

The risk of loading unused post meta is just as likely when loading another ID from the queue as it is when loading an ID that was not already in the queue. However, I don't feel strongly about this being a blocker to an initial commit. If we find that only clearing the queue when a lazy loaded ID is called results in more unnecessary DB calls, then we can adjust this before the final release.

[audrasjb]

As per today's bug scrub: It appears this ticket is still under discussion. As 6.9 is very close, I'm moving it to 6.9. Feel free to move it back to 6.8 if it can be committed by Monday.