[wp-trac] [WordPress Trac] #36824: do_all_pings function queries all posts

[WordPress Trac]

#36824: do_all_pings function queries all posts
-------------------------------------------------+-------------------------
 Reporter:  spacedmonkey                         |       Owner:
     Type:  enhancement                          |  boonebgorges
 Priority:  normal                               |      Status:  reviewing
Component:  Pings/Trackbacks                     |   Milestone:  Future
 Severity:  normal                               |  Release
 Keywords:  has-unit-tests 2nd-opinion needs-    |     Version:  2.1
  patch                                          |  Resolution:
                                                 |     Focuses:
                                                 |  performance
-------------------------------------------------+-------------------------
Changes (by boonebgorges):

 * keywords:  has-patch has-unit-tests 2nd-opinion => has-unit-tests 2nd-
     opinion needs-patch
 * milestone:  4.7 => Future Release


Comment:

 Hi @spacedmonkey and @dshanske - Thanks for working on this ticket. A few
 comments about [attachment:36824.4.diff].

 The 'meta' portion of the `_pingme` and `_encloseme` queries is incorrect.
 It should be `'meta_key' => '_pingme'`, not `'meta_value'`.

 Switching to `get_posts()` with `posts_per_page=-1` feels dangerous to me.
 Currently, `do_all_pings()` requires many database queries, but each one
 will only load a small amount of content into memory. I can guarantee that
 loading `post_content` for hundreds or thousands of posts in a single
 query is going to cause out-of-memory fatal errors. We can still leverage
 the post cache when available by doing `fields=ids` and then, in the
 foreach loop, calling `get_post()`. (This is similar to what's already
 happening with the `do_trackbacks()` loop.)

 Adding a parameter to `WP_Query` that checks 'to_ping' seems OK to me. But
 it's worth noting that changing to `WP_Query` doesn't fix the "bad query"
 issue reported by New Relic. If anything, it'll make it worse, since the
 SQL query generated by `WP_Query` is bound to be more complex. Also,
 `to_ping` acts like a boolean in your patch, but the `to_ping` column is
 not boolean - it's a list of URLs. A more accurate and robust parameter
 might be something like:

 {{{
 if ( null !== $q['has_to_ping'] ) {
     if ( $q['has_to_ping'] ) {
          $where .= " AND {$wpdb->posts}.to_ping <> ''";
     } else {
          $where .= " AND {$wpdb->posts}.to_ping = ''";
     }
 }
 }}}

 One final thought. If we're really concerned about `to_ping` performance,
 we might consider moving/mirroring `to_ping` in postmeta. Meta queries
 like this are quite fast, because they only touch the indexed `meta_key`
 column: `SELECT ... WHERE m.meta_key = 'to_ping'`. It would be valuable to
 see performance benchmarks.

 There are a couple improvements here, any of which could be considered for
 4.7 as a standalone item if an updated patch were ready in time:
 - a param for `WP_Query` that references `to_ping`
 - use `get_posts( 'fields=ids' )` + `get_post()` for `_pingme` and
 `_encloseme` queries
 - mirror `to_ping` in postmeta and use it instead of the `to_ping` column
 when querying for posts that need to send trackbacks

--
Ticket URL: <https://core.trac.wordpress.org/ticket/36824#comment:16>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform