WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#24958 assigned defect (bug)

Large number of revisions cause memory exhaustion

Reported by: jshreve Owned by: adamsilverstein
Milestone: Future Release Priority: low
Severity: normal Version: 3.6
Component: Revisions Keywords: has-patch dev-feedback
Focuses: Cc:

Description

This may be a edge case, but if you have a a large number of revisions, a number of things can break.

Right now the post in question has about 1,055 revisions.

Noticeably, two things are happening:

Calling wp_update_post from a plugin fails with the following error:

PHP Fatal error: Allowed memory size of 41943040 bytes exhausted (tried to allocate 16384 bytes) in /path/wp-includes/wp-db.php on line 1228

Using the new revisions API to view earlier revisions returns

"Sorry, something went wrong. The requested comparison could not be loaded." and the error log contains the same memory exhaustion error.

I can seem to update from the post edit screen.

This is happening with the latest stable of 3.6 and only began happening after an update, so I suspect it's something in the new revision core/api. I haven't had a huge amount of time to investigate but my guess is it's trying to load too much revision data at one time? Maybe it should only try to load X amount of revisions at once?

Attachments (3)

24958.diff (602 bytes) - added by nacin 5 years ago.
24958.2.diff (1.3 KB) - added by adamsilverstein 3 years ago.
24958.3.diff (861 bytes) - added by adamsilverstein 3 years ago.

Download all attachments as: .zip

Change History (21)

#1 in reply to: ↑ description @jshreve
5 years ago

Replying to jshreve:

Using the new revisions API to view earlier revisions returns

I meant to say "new revisions UI" here.

#2 follow-up: @adamsilverstein
5 years ago

Wow, 1000+ revisions - that does seem like an edge case... none the less, crashing PHP is bad form and I agree we need some checks in place to make sure we don't do that. I'm not sure about the first issue - the second one may be caused because the revisions page tries to pre-load all of the initial revision comparisons (so you can rapidly scroll thru them). The code is already capable of lazy-loading the data, and in fact thats what happens in the 'compare any two revisions' mode, we should be able to leverage that for large data sets like yours.

We should probably only pre-load a set maximum number, then lazy load the rest. In our testing for 3.6 we tried posts with several hundred revisions, but never got to 1000!

#3 @adamsilverstein
5 years ago

  • Cc adamsilverstein@… added

#4 @nacin
5 years ago

  • Milestone changed from Awaiting Review to 3.6.1

wp_update_post() triggers save_post which calls wp_save_post_revision() which calls wp_get_post_revisions() to get the last revision.

We should set up wp_get_post_revisions() to be able to request the last non-autosave in a single query, rather than potentially loading all revisions into memory. I almost did this in 3.6 for wp_get_post_autosave() because it used to do a direct WP_Query for an autosave, and now it calls wp_get_post_revisions(), which can be painful.

This should fix the issue of wp_update_post(). It won't fix the revisions screen, which *does* have some failover if it takes too long (or too much memory) to process all of the diffs, but it does still try to load all revisions at once. We should place a limit on that actual query, say 200, to force it to occur in chunks. Our JS will handle it automatically.

#5 in reply to: ↑ 2 ; follow-up: @jshreve
5 years ago

Replying to adamsilverstein:

We should probably only pre-load a set maximum number, then lazy load the rest. In our testing for 3.6 we tried posts with several hundred revisions, but never got to 1000!

The reason this was surfaced is because we have a test suite for the WordPress.com REST API that edits the same post over and over - therefor it quickly got to 1000+ revisions :). We could/should probably clear out the data after the tests run for certain things - but that's how it got so high.

#6 in reply to: ↑ 5 @adamsilverstein
5 years ago

Replying to jshreve:

Replying to adamsilverstein:

We should probably only pre-load a set maximum number, then lazy load the rest. In our testing for 3.6 we tried posts with several hundred revisions, but never got to 1000!

The reason this was surfaced is because we have a test suite for the WordPress.com REST API that edits the same post over and over - therefor it quickly got to 1000+ revisions :). We could/should probably clear out the data after the tests run for certain things - but that's how it got so high.

Tearing down your old test data would make sense, also you can limit the number of revisions WordPress stores, see wp_revisions_to_keep. In any case, we should still address the issues you raise in this ticket.

#7 @SergeyBiryukov
5 years ago

  • Version set to 3.6

#8 follow-up: @nacin
5 years ago

My comment was pretty in depth, which shouldn't make it too difficult for someone to come up with a patch, but it is still fairly complicated, and might require some enhancements to WP_Query or a less-than-triumphant return of _wp_get_post_autosave_hack() to do some hijinks. Or, a direct query.

For 3.6.1 I think we should either do @ini_set( 'memory_limit', apply_filters( 'admin_memory_limit', WP_MAX_MEMORY_LIMIT ) ); or do 'posts_per_page' => 1000.

That said, WordPress.com is running an opcode cache (i.e. APC) which means the actual memory of the current process is possibly lower than what might be typical in the non-opcode admin (mid 20s MB). (This might not be true since WP.com runs a metric ton of extra code.) We should test how much memory 1000 revisions actually requires (and compare to both frontend and admin) and see if limiting to 1000 makes sense.

I'm fine with limiting to 1000 (if that's not too high) as a "dude, seriously?" number to avoid fatal errors. And it is, of course, filterable.

#9 in reply to: ↑ 8 @SergeyBiryukov
5 years ago

Replying to nacin:

We should test how much memory 1000 revisions actually requires (and compare to both frontend and admin) and see if limiting to 1000 makes sense.

  1. Created 1000 revisions (in trunk, PHP 5.2.14, Windows) using ocean90's script.
  2. On the Revisions screen, Debug Bar shows "Memory Usage: 34 346 816 bytes".
  3. The same install shows 18 804 216 bytes on the front-end, and 25 544 016 bytes on Dashboard screen.

@nacin
5 years ago

#10 @nacin
5 years ago

  • Keywords has-patch commit added

Thanks, Sergey! OK, so this is something we might only hit if we also have a ton of plugins and creep close to the 40 MB limit. I am actually very surprised WP.com isn't either far above that, or deliberately -1.

24958.diff implements a soft 1000 limit. I say "soft" because you can change it to -1 via $args for wp_get_post_revisions(). It is not enforced and thus does not need to be changed by reaching into a WP_Query filter. I think this is good for 3.6.1.

#11 @nacin
5 years ago

  • Keywords commit removed

1000 is lame for when order => ASC, which we do in a few cases; and I don't actually see a change in handling that from 3.5.

#12 @nacin
5 years ago

  • Milestone changed from 3.6.1 to 3.6.2
  • Priority changed from normal to low

#13 @johnbillion
5 years ago

  • Milestone changed from 3.6.2 to Future Release

After discussion at WCEU we're punting this due to edge case scenario and no immediate fix available.

#14 @fliespl
4 years ago

I have been able to use up 512MB of memory with clean version of wordpress 3.8.X (no plugins loaded).

In my case post_content length was 300KB and there were about 1000 revisions.

I believe that this functionality shouldn't be querying full posts (since "metabox" only displays a list of revisions and not content) unless specified otherwise. Disabling post_content from this query resolved my memory issues. And WP was using less than 128MB.

#15 @adamsilverstein
3 years ago

  • Keywords dev-feedback added

24958.3.diff Limits the query for wp_get_post_revisions in wp_save_post_revision where we are looking for the last non autosave revision... Now we only look back 100 revisions to find the last non autosave (or create a new one anyway).

This should fix the out of memory error in wp_update_post

Noticed that wp_get_post_autosave uses the same code, limiting here has other issues, but note that this function will still consume memory trying to select all revisions at once.

For the issue on the revisions screen itself, the initial bootstrap build in wp_prepare_revisions_for_js is selecting all the revisions to get their meta, we would have to adjust how this is built for large numbers of revisions, or not provide the meta at run time, only the IDs - this would allow for an ID only query which would consume far less memory. The JS could handle this dynamic meta loading after some adjusting. Searching core, thats it for use of wp_get_post_revisions that aren;t already limited, other than the meta-box, which I believe we are removing anyway.

#16 @pdfernhout
3 years ago

Issue #34560 overlaps this one, but is not the same. The difference is the other issue is about server errors resulting from using the edit screen on a large post with many revisions, whereas this issue is about failures resulting in other components from that situation. That other issue also lists use cases for having many edits of a long page.

For both issues, the failures seem to result from using wp_get_post_revisions to get all revision data including content instead of using more selective SQL that just retrieves the needed metadata (or possibly just the count of revisions for the editor). While all systems have their limits, using wp_get_post_revisions as-is makes WordPress hit limits much sooner than otherwise would happen.

Such use of wp_get_post_revisions as-is also slows responsiveness due to unneeded copying of post content which can be substantial sometimes. For example, even if you have enough memory, if you have 100 revisions of a page that are 100K each, WordPress would retrieve 10MB from the database to return perhaps 10K or so of metadata to the user for a list of revisions (or only just a few bytes of metadata in the case of the editor which needs just the total number of revisions). The more revisions and the bigger they are, the worse the unnecessary performance hit is.

It is possible there may be common functionality that could be created to improve both issues, such as an extra option to wp_get_post_revisions to just retrieve post metadata (so, not including post content).

#17 @adamsilverstein
7 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#18 @pembo13
3 months ago

I'm still seeing this issue, WordPress version 4.9.4 Same error:

PHP Fatal error:  Allowed memory size of 268435456 bytes exhausted (tried to allocate 81920 bytes)

Checking the revisions:

SELECT COUNT(*) FROM `wp_posts` WHERE post_parent = '1978' AND post_type = 'revision' 

Yields 537.

Looking at $revisions = wp_get_post_revisions( $post_ID ); it seems like it's loading the entire post, but only information to generate the list are needed (ID, post_author, post_date, post_title). Can we just reduce the fields that are being loaded?

The database really doesn't mind 500+ rows, the issue is the PHP code trying to load all 500 posts in their entirety.


Stack Trace:

Fatal error:  Allowed memory size of 268435456 bytes exhausted (tried to allocate 81920 bytes) in /var/www/wordpress/example.com/wp-includes/wp-db.php on line 1889, referer: https://example.com/wp-admin/edit.php?post_type=page
Stack trace:, referer: https://example.com/wp-admin/edit.php?post_type=page
   1. {main}() /var/www/wordpress/example.com/wp-admin/post.php:0, referer: https://example.com/wp-admin/edit.php?post_type=page
   2. include() /var/www/wordpress/example.com/wp-admin/post.php:174, referer: https://example.com/wp-admin/edit.php?post_type=page
   3. wp_get_post_revisions() /var/www/wordpress/example.com/wp-admin/edit-form-advanced.php:231, referer: https://example.com/wp-admin/edit.php?post_type=page
   4. get_children() /var/www/wordpress/example.com/wp-includes/revision.php:453, referer: https://example.com/wp-admin/edit.php?post_type=page
   5. get_posts() /var/www/wordpress/example.com/wp-includes/post.php:445, referer: https://example.com/wp-admin/edit.php?post_type=page
   6. WP_Query->query() /var/www/wordpress/example.com/wp-includes/post.php:1704, referer: https://example.com/wp-admin/edit.php?post_type=page
   7. WP_Query->get_posts() /var/www/wordpress/example.com/wp-includes/class-wp-query.php:3230, referer: https://example.com/wp-admin/edit.php?post_type=page
   8. wpdb->get_results() /var/www/wordpress/example.com/wp-includes/class-wp-query.php:2831, referer: https://example.com/wp-admin/edit.php?post_type=page
   9. wpdb->query() /var/www/wordpress/example.com/wp-includes/wp-db.php:2488, referer: https://example.com/wp-admin/edit.php?post_type=page
  10. mysqli_fetch_object() /var/www/wordpress/example.com/wp-includes/wp-db.php:1889, referer: https://example.com/wp-admin/edit.php?post_type=page

Note: See TracTickets for help on using tickets.