Make WordPress Core

Opened 12 years ago

Last modified 12 days ago

#24688 new defect (bug)

Memory exhaustion caused by very many unattached media

Reported by: andy's profile andy Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch has-test-info reporter-feedback
Focuses: performance 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)

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

Download all attachments as: .zip

Change History (21)

@andy
12 years ago

#1 @SergeyBiryukov
12 years ago

  • Component changed from General to Media

Related: #23044

#2 @nacin
12 years ago

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

#3 @SergeyBiryukov
12 years ago

  • Version changed from trunk to 2.5

Related: [7222]

#4 @wonderboymusic
12 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#5 @ocean90
12 years ago

  • Milestone changed from 3.7 to Future Release

Punting due to lack of testing.

#6 in reply to: ↑ description @ericlewis
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: @kitchin
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 @andy
11 years ago

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

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

#9 @chriscct7
10 years ago

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

@dllh
9 years ago

Updated patch

#10 @dllh
9 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
9 years ago

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

#12 @iCaleb
7 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.

@iCaleb
7 years ago

Updated patch w/ previously mentioned changes.

@david.binda
7 years ago

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

#13 @david.binda
7 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 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.

This ticket was mentioned in Slack in #core by sirlouen. View the logs.


3 weeks ago

This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.


3 weeks ago

This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.


12 days ago

#17 @SirLouen
12 days ago

  • Focuses performance added
  • Keywords has-test-info reporter-feedback added; needs-testing removed

Reproduction Report

Description

❌ This report can't validate that the issue can be reproduced.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Fourteen 4.2
  • MU Plugins: None activated
  • Plugins:
    • Bulk Image Generator 1.0.0
    • Image Memory Exhaustion Tester 1.0.0
    • Test Reports 1.2.0

Test Reproduction Suite

  1. First, I have created 2000 images for the test. It is easy to extrapolate results to 30K but the time it takes to do the whole process is unnecessary in the memory consumption doesn't scale.
  1. Secondly I'm generating a total of 2000 images for the test as I said with the previous plugin
  2. Finally, I had to create an ad-hoc code to test this specific scenario: https://raw.githubusercontent.com/SirLouen/image-memory-exhaustion-tester/refs/tags/1.0.0/image-memory-exhaustion-tester.php
  3. It's very important to increase in the PHP server, the max_execution_time, put a good number, like 3000 seconds or something like that. Depending on your computer running through 2000 images can take a while
  4. Run the memory tester plugin
  5. ❌ No special memory exhaustion problems allocated (attached logs in Supplemental Artifacts section)

Actual Results

  1. ❌ Error condition is not occurring

Additional Notes

  • Please review steps taken to verify if I'm not taking something into consideration, or if any of my plugins (specially the memory testing part), is doing what is being reported. Occasionally, it's very difficult to reproduce the same scenario in the same way that the reporter is running it. This is why I've been recommending for quite a while now that reporters, specially when they report any specific performance issues, to provide code samples or whatever Testing Use-Case they think could be valid
  • @davidbinda @iCaleb, how were you able to test your patches?
  • Given that this report is so old, maybe it was solved at some point in between.

PS: Generally I don't spend so much time creating so much code to prove something. But I found that this was a great example report I want to also add to my Testing Use-Case article in the performance section.

Supplemental Artifacts

=== Memory Exhaustion Test Results ===
Start Time: 2025-06-02 20:24:39
Initial Memory Usage: 36 MB
Initial Peak Memory: 36 MB
--- TEST 1: get_children() Function ---
Attachments Found: 2010
Execution Time: 0.1073 seconds
Memory Increase: 14 MB
Peak Memory Increase: 14 MB

--- TEST 2: previous_image_link() Chain (Simulating Unattached Attachment Page) ---
Found 2010 unattached images to test
Progress: 100 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 200 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 300 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 400 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 500 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 600 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 700 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 800 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 900 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1000 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1100 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1200 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1300 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1400 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1500 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1600 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1700 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1800 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1900 images processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 2000 images processed, Current Memory: 52 MB, Peak: 52 MB
Previous Image Link Calls: 2010
Unattached Images Tested: 2010
Execution Time: 66.2879 seconds
Memory Increase: 2 MB
Peak Memory Increase: 2 MB

--- TEST 3: Direct adjacent_image_link() with post_parent=0 Simulation ---
Progress: 50 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 100 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 150 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 200 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 250 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 300 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 350 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 400 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 450 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 500 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 550 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 600 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 650 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 700 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 750 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 800 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 850 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 900 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 950 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1000 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1050 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1100 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1150 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1200 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1250 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1300 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1350 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1400 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1450 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1500 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1550 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1600 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1650 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1700 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1750 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1800 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1850 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1900 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 1950 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Progress: 2000 attachments processed, Current Memory: 52 MB, Peak: 52 MB
Adjacent Image Link Calls: 4020
Attachments Tested: 2010
Execution Time: 133.7525 seconds
Memory Increase: 0 B
Peak Memory Increase: 0 B

--- OVERALL RESULTS ---
Total Execution Time: 200.1479 seconds
Final Memory Usage: 52 MB
Final Peak Memory: 52 MB
Total Memory Increase: 16 MB
Total Peak Memory Increase: 16 MB

PHP Memory Limit: 256M
Memory Usage: 20.31% of limit
Note: See TracTickets for help on using tickets.