Make WordPress Core

Opened 12 years ago

Last modified 6 years ago

#23336 reopened defect (bug)

Sticky Posts lack sanity bounding. If used too much, can severely degrade performance.

Reported by: markjaquith's profile markjaquith Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: needs-patch
Focuses: Cc:

Description

Came across an issue where a site was using sticky posts for a slider on the front page. The rest of the front page used custom taxonomy queries. As such, they'd mark items as sticky, have the slider grab the first three. Over years, they accumulated thousands of sticky posts, which, as you can imagine, made the front page of their site absurdly slow, as it was querying those thousands of posts every time.

Should we establish some sort of sanity limit here, to keep people from shooting themselves in the foot? I found this a REALLY hard issue to diagnose. Even with debug bar, there is no indication that sticky posts are being queried. There's just a giant WP_Query call that does a giant IN() query.

Something like a limit of 100 with FIFO would keep things from getting too crazy, without restricting people too much. If you have a need for more than that, you need to be using a taxonomy query.

Attachments (5)

23336-sample-posts.zip (78.0 KB) - added by iandunn 12 years ago.
23336.diff (776 bytes) - added by iandunn 12 years ago.
23336.2.diff (781 bytes) - added by iandunn 12 years ago.
23336.3.diff (1.9 KB) - added by mbijon 12 years ago.
Bases sticky post limit off site's posts_per_page option
Screen Shot 2014-05-04 at 6.40.03 PM.png (29.5 KB) - added by wonderboymusic 11 years ago.

Download all attachments as: .zip

Change History (31)

@iandunn
12 years ago

#1 @iandunn
12 years ago

  • Cc ian_dunn@… added
  • Keywords has-patch added

I've attached a WRX with 5k sticky posts to make the speed problem obvious on local installs. For me, the front page loaded in 600 ms with none of the posts set to sticky, and 11 seconds with all of them sticky. I'm testing with a front-page.php that only calls get_header() and get_footer(), so only the main query is running.

I've also attached a first attempt at a patch. It just truncates the $sticky_posts array in get_posts() right after they're pulled from the DB, so that only the first 100 make it into the query. With the patch applied, the page load time drops back down to under a second.

I wrapped the 100 value in a new filter called "sticky_posts_query_limit".

When I get some more time, I still want to dig through the code a bit more and see if there are other situations that need a similar modification, or if there are any unintended consequences to this approach. It at least passes the query unit tests, though.

#2 @iandunn
12 years ago

I looked through all the times sticky_posts is pulled from the DB, and couldn't reproduce any other speed issues in those areas, so it seems like get_posts() was the only spot that triggered the problem.

The array_slice() call that the patch adds to get_posts() is in a condition that only runs on the home page, so I can't think of any side effects outside of the query that the patch targets, and haven't run across any in testing.

So, I think the patch is ready unless anyone can think of something I've missed.

#3 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.6

#4 @kovshenin
12 years ago

  • Cc kovshenin added

#5 @mbijon
12 years ago

  • Keywords reporter-feedback added

Came across this after researching stickies as part of #12702 (it's related)

Short version of what I posted there:
Stickies are ridiculous as-is, they need a refactor.


In #12702 I suggested stickies should be refactored to be a taxonomy or similar. The patch here reinforces that for me:

I really don't like the idea of hard-limiting stickies to 100 in core. The possibility that this will break user expectations (and existing sites) is really high (not many of them, but definitely someone's).

We definitely should have stickies follow the same conventions as every other content & option-type in the DB. I see two options to get there:
(a) deprecating existing stickies & telling the community to use a taxonomy
(b) upgrading stickies to a built-in taxonomy & then adding code to the upgrade process that can detect & parse the existing sticky array into the new taxonomy

Any preferences? I like option (b) -- if there's any support for it then I can probably grab @sphoid from ticket #12702 & work on that code (Shy about working that deep unless there's support)

#6 @mboynes
12 years ago

  • Cc mboynes@… added

#7 @iandunn
12 years ago

mbijon, the patch only limits how many sticky posts will be displayed on the home page, and there's a filter around the value so people can easily change it. It doesn't effect anything else regarding creating or using sticky posts.

Does that still pose a concern for you? If so, can you give an example use case where someone would want to show 100+ posts on the home page? If there are some, maybe the default limit should be relaxed to 175 or even 250. Given the bug markjaquith describes, though, that still might not be enough reason to remove the limit completely.

I agree that the way sticky posts are currently managed isn't great, but that's kind of tangential to this ticket. If #12702 was going to be committed soon, then that would make this one irrelevant, but it doesn't seem like there's anything close to a consensus there, and it's definitely not gonna make it into 3.6. So, I would think it'd be a good idea to go ahead with this ticket to fix the current bug, rather than waiting for a comprehensive refactoring of sticky posts to happen.

#8 @mbijon
12 years ago

  • Cc mike@… added

Odds are you and Mark are right that a default limit is OK. Stickies have always had no limit, so most themes or sites are already managing them manually ... or have just gotten used to things being slow & ugly.

At least this is a filter & will get documented. With 3.6 launch docs, people will be able to find it easier than Mark was able to track down his performance issue.

Is it worth making this filter homepage-specific though? A bit long, but unless other things get the same filter, this is more of a 'sticky_posts_is_home_query_limit'

@iandunn
12 years ago

#9 @iandunn
12 years ago

That's a good point. I've updated the filter name to sticky_posts_home_query_limit.

#10 @mbijon
12 years ago

Tks.

+1 to this in 3.6. I've tested & it's working as-expected

(note to self, should build a unit test...)

#11 follow-up: @mboynes
12 years ago

I agree with mbijon and I'm not a fan of an arbitrary static number like 100. Why not use the posts_per_page site option instead?

#12 in reply to: ↑ 11 @iandunn
12 years ago

Replying to mboynes:

I'm not a fan of an arbitrary static number like 100. Why not use the posts_per_page site option instead?

It's not exactly arbitrary. It's was chosen because it's large enough that it won't affect the vast majority of sites, and because there doesn't seem to be a reasonable use case for displaying 100+ posts on the home page. While you can draw a logical connection between posts_per_page and the query limit, it's set to 10 by default, so using it would almost certainly result in an unacceptable number of sites being affected.

The goal here is to just take care of a rare edge case, without affecting normal sites. 100 is probably large enough to do that, but 10 is way too small.

@mbijon
12 years ago

Bases sticky post limit off site's posts_per_page option

#13 @mbijon
12 years ago

@mboynes, great idea.

I grab the site's posts_per_page option & * 10: attachment:23336.3.diff

#14 follow-up: @mboynes
12 years ago

@iandunn, I would offer that it's the rare edge case site which needs more sticky posts than displays posts per page. And in that edge case, they're doing it wrong.

Just sit back and think about it, and I think you'll see that it makes sense. What's the purpose of "Sticky Posts"? To force posts to the top of the main loop, right? If you only want to display 10 posts per page (or maybe you up it to 20 or so), why would you have 50 sticky posts? Better still, lets say you have 10 posts per page and you have 15 sticky posts -- wouldn't you wonder why there were 15 posts on your homepage, and think that was a bug? If anything, what this suggests to me is that sticky posts should be woven into the pagination of the loop, but at that point, you'd want to overhaul it like mbijon suggests, using a taxonomy.

@mbijon, your solution is clever. I think it might open up a gate to big and ugly numbers if someone increases their posts per page (e.g. 50 means 500 sticky posts). I really think that it should just be restricted to posts per page.

#15 in reply to: ↑ 14 ; follow-up: @iandunn
12 years ago

Replying to mboynes:

@iandunn, I would offer that it's the rare edge case site which needs more sticky posts than displays posts per page. And in that edge case, they're doing it wrong.

I agree that they're doing it wrong, but it's not uncommon for users to forget to un-stick posts that they don't want to be sticky anymore, and just because someone's making a mistake doesn't mean WP can't handle the situation gracefully.

In this case, I think the bottom line is that using a number like 10 is going to cause problems for a significantly larger number of people than 100 will.


Better still, lets say you have 10 posts per page and you have 15 sticky posts -- wouldn't you wonder why there were 15 posts on your homepage, and think that was a bug?

I don't think that's really what's happening here. Sticky posts don't count against the posts_per_page limit in the loop, they're added to it. So, in that hypothetical situation, the user would already be seeing 25 posts on the page, and that behavior wouldn't change as a result of my patch.

The reason the user didn't notice in @markjaquith's case is because their template was only displaying the first 3 posts from the loop, even though it contained 2,000+ posts.

The limit only comes into play when the number of sticky posts exceeds it:

posts_per_page = 10; 3000 posts in database, 15 are sticky; result = 25 posts displayed
posts_per_page = 10; 3000 posts in database, 75 are sticky; result = 85 posts displayed
posts_per_page = 10; 3000 posts in database, 150 are sticky; result = 110 posts displayed
posts_per_page = 10; 3000 posts in database, 1500 are sticky; result = 110 posts displayed
posts_per_page = 10; 3000 posts in database, 2500 are sticky; result = 110 posts displayed

#17 @mboynes
12 years ago

@SergeyBiryukov thanks for that article. Wow, that is very confusing.

I'm starting to think that a temporary solution isn't the way to go here. Perhaps this should be pushed to 3.7 with a request for a significant overhaul.

#18 @aaroncampbell
12 years ago

  • Milestone changed from 3.6 to Future Release

Since this isn't new and I don't think the offered solutions are really what we want, I'm moving this out of 3.6

#19 @mbijon
12 years ago

I agree with the move to a future release. Since devs on sites big enough to have problems can & do avoid this perf issue by creating their own stickies taxonomy, this doesn't seem important enough to delay 3.6 on.

For future discussion though: Could we make stickies filterable so that a plugin could divert them to a taxonomy, or should we replace core's current serialized list of sticky IDs with a taxonomy? The latter might require some upgrade code that could migrate the serialized list into the new core taxonomy.

(I think that ideally, doing both make sense if there's people with time for it)

#20 @wonderboymusic
12 years ago

  • Keywords close added

Stickies could of course use a rethink, or even some management tools in the admin, but short of that, I don't see a great way to "fix" the issue of someone making a bad architectural decision

#21 @wonderboymusic
11 years ago

  • Keywords stickies added

#22 @wonderboymusic
11 years ago

  • Keywords needs-refresh added; reporter-feedback close removed

If there are any stickies, I think there should be a list table view of them via an admin link (under Posts?). If you have hundreds (or 1000s) of posts, it can be annoying trying to find all stickies.

#23 @mbijon
11 years ago

That's actually a great idea. Do we need a separate list, or could we just add a filter at the top of the existing Posts list (/wp-admin/edit.php)?

OTOH, I think a filter like that is going to have performance problems unless we get stickies out of their serialized format. It may also be a feature-request that deserves its own ticket.

#24 follow-up: @wonderboymusic
11 years ago

  • Keywords close added; needs-refresh removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Added a screenshot, there is already a list table view for stickies indicating the count. What more do we really need here?

#25 in reply to: ↑ 24 @helen
11 years ago

  • Keywords close removed
  • Milestone set to Future Release
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Replying to wonderboymusic:

Added a screenshot, there is already a list table view for stickies indicating the count. What more do we really need here?

Sanity bounding in the query.

#26 @chriscct7
9 years ago

  • Keywords needs-patch added; has-patch stickies removed
Note: See TracTickets for help on using tickets.