WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 13 months ago

#24688 new defect (bug)

Memory exhaustion caused by very many unattached media

Reported by: andy Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

A blog with over 30,000 unattached images was exhausting available memory on unattached attachment pages. The exhaustion occurred in previous_image_link() -> adjacent_image_link() -> get_children() due to the large number of rows selected with post_parent=0.

This is not the only place where get_posts() might be called with post_parent=0. A similar query appears in twentythirteen_the_attached_image() but the effect is wisely mitigated by fields=ids. Earlier, twentyten/loop-attachment.php called get_children() with post_parent=0 on unattached image pages which is horribly inefficient when there are many unattached images.

Attached patch depends on #24687. The change to adjacent_image_link() creates a MySQL query which selects only the desired row. This fixes the memory exhaustion error on the blog with 30,000 unattached images. The once-only filters would be unnecessary if WP_Query accepted arbitrary extra WHERE strings.

Attachments (4)

24688.diff (2.4 KB) - added by andy 6 years ago.
media.patch (3.0 KB) - added by dllh 3 years ago.
Updated patch
24688-2.diff (3.3 KB) - added by iCaleb 13 months ago.
Updated patch w/ previously mentioned changes.
24688-3.diff (4.8 KB) - added by david.binda 13 months ago.
Adding basic unit tests for the previous_image_link() and next_image_link() functions.

Download all attachments as: .zip

Change History (17)

@andy
6 years ago

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Media

Related: #23044

#2 @nacin
6 years ago

  • Keywords has-patch 3.7-early added
  • Milestone changed from Awaiting Review to Future Release

#3 @SergeyBiryukov
6 years ago

  • Version changed from trunk to 2.5

Related: [7222]

#4 @wonderboymusic
6 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#5 @ocean90
6 years ago

  • Milestone changed from 3.7 to Future Release

Punting due to lack of testing.

#6 in reply to: ↑ description @ericlewis
5 years ago

Replying to andy:

The once-only filters would be unnecessary if WP_Query accepted arbitrary extra WHERE strings.

That's interesting. Are you subtly proposing this idea?

#7 follow-up: @kitchin
5 years ago

About the patch
return "$where AND (...
should be
return "( $where ) AND (...
There are operators that bind less tightly, like OR.

Is that what you meant by testing? :)

#8 in reply to: ↑ 7 @andy
5 years ago

I think since #26042 is closed this ticket now just needs a new patch and testing.

Last edited 5 years ago by andy (previous) (diff)

#9 @chriscct7
4 years ago

  • Keywords needs-patch added; has-patch 3.7-early removed

@dllh
3 years ago

Updated patch

#10 @dllh
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I've updated the patch for trunk and given it a test drive. It seems to work well for me. Any chance of nudging this along?

#11 @kitchin
3 years ago

My objection above still stands, to $where without parentheses in return "$where AND ...

#12 @iCaleb
13 months ago

Found an issue with the query for retrieving the next attachment link. It will always link to the last image attached to the post, not necessarily the next one. Turns out, 'orderby' => "menu_order $order, ID $order" will result in ORDER BY wp_posts.menu_order DESC, wp_posts.ID DESC no matter what $order is (it'll never be ASC). This can be solved by instead passing an array into the orderby param as supported since WP 4.0.


Next up, array_pop was also causing a PHP notice:

Notice: Only variables should be passed by reference at wp-includes/media.php:2812

Line 2812 was 'post_parent' => $post->post_parent. This is solved by either passing a single variable into array_pop(), else avoiding using it. Since in this case get_children() is just either going to return a single element array or an empty array, I opted for just accessing the first key directly if it exists.


Lastly, I did apply the return "( $where ) AND ( changes mentioned above, but it results in a SQL syntax error.

@iCaleb
13 months ago

Updated patch w/ previously mentioned changes.

@david.binda
13 months ago

Adding basic unit tests for the previous_image_link() and next_image_link() functions.

#13 @david.binda
13 months ago

My objection above still stands, to $where without parentheses in return "$where AND ...

@kitchin , do you happen to have an example of code which would break by applying the patch the way it's being proposed here?

My understanding is that the WHERE clause in WP_Query::get_posts where posts_where filter is being applied is being built as a chain of AND clauses. In case a plugin would be filtering posts_where while adding OR w/o proper parentheses, it would either have to be doing some weird string modifications (as the $where passed to the callback starts with AND, eg.: AND wptests_posts.post_parent = 85 AND (wptests_posts.post_mime_type LIKE 'image/%') AND wptests_posts.post_type = 'attachment' AND ((wptests_posts.post_status = 'inherit')) and is being appended to ... WHERE 1=1 string, eg.: SELECT wptests_posts.ID FROM wptests_posts WHERE 1=1 ), or would be appending OR to the end of a string of ANDs effectively bypassing all the previous conditions.

That said, I don't think the callback proposed here should really wrap the initial part of the $where into parenthesis, especially as it would require us replacing the leading AND by AND ( which really feels wrong and error prone. In case something would get wrong by not using parenthesis there, it would be because some plugin code does it wrong.

Note: See TracTickets for help on using tickets.