Opened 12 years ago
Last modified 6 years ago
#24688 new defect (bug)
Memory exhaustion caused by very many unattached media
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 2.5 |
Component: | Media | Keywords: | has-patch needs-testing |
Focuses: | Cc: |
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)
Change History (17)
#2
@
12 years ago
- Keywords has-patch 3.7-early added
- Milestone changed from Awaiting Review to Future Release
#6
in reply to:
↑ description
@
11 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:
↓ 8
@
11 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
@
11 years ago
I think since #26042 is closed this ticket now just needs a new patch and testing.
#10
@
8 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
@
8 years ago
My objection above still stands, to $where
without parentheses in return "$where AND ...
#12
@
6 years 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.
@
6 years ago
Adding basic unit tests for the previous_image_link() and next_image_link() functions.
#13
@
6 years 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 AND
s 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.
Related: #23044