Skip to:
Content

bbPress.org

Opened 7 years ago

Last modified 3 years ago

#1925 new enhancement

Optimize `_bbp_last_active_time` to use `post_modified` instead

Reported by: vibol Owned by:
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.1.2
Component: General - Performance Keywords: early
Cc: vibol, georgemamadashvili@…, wordpress@…, jared@…, stephen@…, nashwan.doaqan@…, erick@…, pippin@…, scott@…

Description

Background ticket: http://bbpress.trac.wordpress.org/ticket/1885

Background Conversation:

Vibol: For the first query, last_active_time now looks to be the culprit. It's a postmeta lookup with a sort.

JJJ: Meta queries are always going to be a slow like this. There isn't an available index we can use here to avoid the meta query. We *could* hack in repurposing post_date as the freshness time, but we lose the actual date the topic was posted. It would take some rearchitecting to move the original post creation date to postmeta, to make the index available.

Problem: The _bbp_last_active_time query is doing a full table scan and sort against the postmeta table when retrieving forum index pages. The slowdown is linear with size of table scan. Here's an example slow query:

# Time: 120804 19:52:17
# Query_time: 21.460766  Lock_time: 0.000051 Rows_sent: 25  Rows_examined: 313770
SET timestamp=1344127937;
SELECT SQL_CALC_FOUND_ROWS  wp_posts.* FROM wp_posts  INNER JOIN wp_postmeta ON (wp_posts.ID = wp_postmeta.post_id) WHERE 1=1  AND wp_posts.post_parent = 17  AND wp_posts.post_type = 'topic' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'closed') AND (wp_postmeta.meta_key = '_bbp_last_active_time' ) GROUP BY wp_posts.ID ORDER BY wp_postmeta.meta_value DESC LIMIT 0, 25;

Expected behavior: Should be fast if the data is indexed, but the postmeta table does not have indexes against it.

Guidance: I'm looking for someone to fix this, or, I am happy to put in the time to fix it with guidance on desired approach. This is a non-trivial issue, so someone with more experience in WP/BBP is ideal.

Attachments (18)

mysql-slow.log (403.2 KB) - added by vibol 7 years ago.
1925.diff (8.7 KB) - added by MZAWeb 7 years ago.
1925.bbPress1.php.diff (624 bytes) - added by netweb 6 years ago.
1925.003.patch (14.6 KB) - added by thebrandonallen 6 years ago.
Read and write post_modified. No database upgrading.
1925-thebrandonallen.png (56.6 KB) - added by netweb 6 years ago.
1925-mzaweb.png (51.4 KB) - added by netweb 6 years ago.
1925-mzaweb-wp_posts.sql (5.4 KB) - added by netweb 6 years ago.
1925-thebrandonallen-wp_posts.sql (7.3 KB) - added by netweb 6 years ago.
1925.04.diff (17.9 KB) - added by netweb 6 years ago.
1925.05.diff (17.9 KB) - added by netweb 6 years ago.
0001-bbPress-performance-enhancements-http-bbpress.trac.w.patch (11.2 KB) - added by vibol 6 years ago.
1925.patch (10.4 KB) - added by johnjamesjacoby 6 years ago.
Includes _update_ functions, removes phpdoc changes from previous patches
1925.2.patch (10.2 KB) - added by netweb 6 years ago.
Fixed Copy Pasta Error ;)
1925.converters.patch (17.3 KB) - added by netweb 6 years ago.
Updated converters:
1925.post_modified.01.diff (17.6 KB) - added by thebrandonallen 6 years ago.
post_modified - with wp_update_post, allows us to pass our own post_modified(_gmt) times, fixes issue with revisions
1925.3.patch (17.8 KB) - added by johnjamesjacoby 6 years ago.
1925.4.diff (25.6 KB) - added by thebrandonallen 6 years ago.
1925.alt.01.diff (8.9 KB) - added by netweb 6 years ago.

Download all attachments as: .zip

Change History (101)

#1 @Mamaduka
7 years ago

  • Cc georgemamadashvili@… added

#2 follow-up: @johnjamesjacoby
7 years ago

So... We're sort of stuck. The approach you quoted from our previous conversation is the only available index in the posts table that makes sense to try to use. We could also, maybe, try a subquery using the ID and post_parent indexes, but that only works if we can guarantee that newer replies will always have a higher ID. This *should* be the case, but it's hard to know for sure given the way the importer works, and how other forum software stores its data.

It's a problem not easily solved due to our limited schema and a lack of indexes on the postmeta table. Happy that you're around looking into it too.

#3 @vibol
7 years ago

As I dig through this, it seems like there are more issues that crop up. See the following set of queries that happen from forum view to thread reply.

# Query_time: 14.640429  Lock_time: 0.000133 Rows_sent: 25  Rows_examined: 313770
SET timestamp=1345373401;
SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  INNER JOIN wp_postmeta ON (wp_posts.ID = wp_postmeta.post_id) WHERE 1=1  AND wp_posts.post_parent = 17  AND wp_posts.post_type = 'topic' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'closed') AND (wp_postmeta.meta_key = '_bbp_last_active_time' ) GROUP BY wp_posts.ID ORDER BY wp_postmeta.meta_value DESC LIMIT 0, 25;
# Time: 120819  5:51:22
# Query_time: 11.424189  Lock_time: 0.000054 Rows_sent: 1  Rows_examined: 1623849
SET timestamp=1345373482;
SELECT COUNT( DISTINCT post_author ) FROM wp_posts WHERE ( post_parent = 1623928 AND post_status = 'publish' AND post_type = 'reply' ) OR ( ID = 1623928 AND post_type = 'topic' );
# Time: 120819  5:51:50
# Query_time: 27.115751  Lock_time: 0.000070 Rows_sent: 55996  Rows_examined: 56000
SET timestamp=1345373510;
SELECT ID FROM wp_posts WHERE post_parent = 17 AND post_status IN ( 'publish', 'closed' ) AND post_type = 'topic' ORDER BY ID DESC;

The last one I'll have to try to attach because it's too big to fit in this field.

@vibol
7 years ago

#4 @MZAWeb
7 years ago

WP posts have four date fields. post_date, post_date_gmt, post_modified and post_modified_gmt. Feels hacky but we could use the post_modified or post_modified_gmt in the forums and topics records for the last_active_time. Even without index, not having to join against postmeta will increase the performance a lot

Last edited 7 years ago by MZAWeb (previous) (diff)

#5 @MZAWeb
7 years ago

  • Cc wordpress@… added

#6 @johnjamesjacoby
7 years ago

Sadly, since all of the core database columns are used by WordPress already, reusing a column for our own purposes is probably dangerous, and will take more work to override the way WordPress naturally writes to those columns than it's probably worth.

post_modified *almost* makes sense, but we should be fully aware of what WordPress core uses that for before hijacking it. If it's used for comparing revisions, we're out of luck; if it's not used at all, we're at risk of it being deprecated and disappearing completely (like post_category).

#7 @johnjamesjacoby
7 years ago

  • Milestone changed from 2.3 to 2.4

#8 @MZAWeb
7 years ago

Query times are the average of 20 runs. First couple of runs ignored as to get MySQL "hot".

DB Size:

forums 5
topics 29208
replies 95755

Main query now (ordering by meta_value):

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM   wp_posts
       INNER JOIN wp_postmeta
               ON ( wp_posts.ID = wp_postmeta.post_id )
WHERE  1 = 1
   AND wp_posts.post_parent = 4
   AND wp_posts.post_type = 'topic'
   AND ( wp_posts.post_status = 'publish'
          OR wp_posts.post_status = 'closed'
          OR wp_posts.post_status = 'private' )
   AND ( wp_postmeta.meta_key = '_bbp_last_active_time' )
GROUP  BY wp_posts.ID
ORDER  BY wp_postmeta.meta_value DESC
LIMIT  0, 15

Avg. time: 162ms

Main query without the join:

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM   wp_posts
WHERE  1 = 1
   AND wp_posts.post_parent = 4
   AND wp_posts.post_type = 'topic'
   AND ( wp_posts.post_status = 'publish'
          OR wp_posts.post_status = 'closed'
          OR wp_posts.post_status = 'private' )
 GROUP  BY wp_posts.ID
ORDER  BY wp_posts.post_modified DESC
LIMIT  0, 15	

Avg. time: 14.9ms

I'm cleaning the patch to switch all uses of _bbp_last_active_time meta to post_modified. Will post asap.

@MZAWeb
7 years ago

#9 follow-ups: @MZAWeb
7 years ago

First pass

1) The batch update script in tools.php is not working. Need to fix.
2) I need to change the importer in includes/admin/converters/bbPress1.php, but I don't have a clue how. Need to research how it works. (Never played with the importer)

#10 @vibol
7 years ago

Finally swinging back around to this. MZAWeb, are you using your changes in production?

Another field that has come to my attention is menu_order. It's primarily used for menu items, but is seldom used for Posts. There are already many plugins that use it to allow users to manually order their Pages and Posts. It's an int(11) so it should be able to fit a timestamp until about 2033.

In either case (post_modified or menu_order), altering the index on the table to add the newly referenced field should help speed things up as well since post_modified and menu_order are not in the indices.

I'd be curious to hear your thoughts.

#11 @vibol
7 years ago

An update.

First off, thank you MZAWeb for contributing your patch. The patch makes a significant difference in performance for my use case, enough so that I intend to use it in production.

The only concern would be curiosity into where WordPress updates last_modified--I believe it does this if a Post is edited (in this case, a Topic). I'm still mulling over the idea of using menu_order, simply because that is a field that is seldom changed by WordPress.

I'll put up changes I made to MZAWeb's patch that prevents an infinite recursion when a Forum is created. I'll also include an index that I've added to further speed up the query.

#12 @jaredatch
7 years ago

  • Cc jared@… added

#13 in reply to: ↑ 9 @netweb
7 years ago

  • Cc stephen@… added

Replying to MZAWeb:

2) I need to change the importer in includes/admin/converters/bbPress1.php, but I don't have a clue how. Need to research how it works. (Never played with the importer)

This part should be pretty straightforward (for me), I'll test your patch and add a patch for the importers here next week.

#14 follow-up: @alex-ye
7 years ago

  • Cc nashwan.doaqan@… added

I really like this ticket it will help to get a better performance , I read your conversation about using last_modified or menu_order but It seems not a good idea for me , And you know why ! ...

There are some other options we can use :
1 - Create a new table "wp_forum_data" like an example ,
with an indexed columns for the most needed things .
2 - Extend the wp_posts table columns .

#15 in reply to: ↑ 14 ; follow-up: @MZAWeb
7 years ago

Replying to alex-ye:

I really like this ticket it will help to get a better performance , I read your conversation about using last_modified or menu_order but It seems not a good idea for me , And you know why ! ...

Actually, I'm not sure why it doesn't seem a good idea for you. Care to elaborate? I'm on the fence myself and would love to hear arguments.

There are some other options we can use :
1 - Create a new table "wp_forum_data" like an example ,
with an indexed columns for the most needed things .
2 - Extend the wp_posts table columns .

I totally get the arguments in favor of that, I really do. But I'm quite sure adding a new table or field it's not gonna happen.

#16 in reply to: ↑ 15 @alex-ye
7 years ago

Replying to MZAWeb:

Care to elaborate? I'm on the fence myself and would love to hear arguments.

The main arguments is :

1 - WordPress is still use the post_modified, post_modified_gmt and menu_order , So using this fields by bbPress in another meaning can cause many bugs as my expectation .

2 - _bbp_last_active_time is not the only meta value which make SQL queries slow , bbPress use many meta fields , and it may be add more in the future

3 - What if the keymaster want to know the really last modified forum ?! , last_active_time and post_modified have a confuse meaning ! , This is not logical to me

I totally get the arguments in favor of that, I really do. But I'm quite sure adding a new table or field it's not gonna happen.

I think adding a new table can be good idea if we have many meta to index , but can you explain why you refuse to add a new column ?

#17 @johnjamesjacoby
7 years ago

  • Milestone changed from 2.4 to 2.5

If someone wants to patch this up to use post_modified, I'd love to get this going for 2.5.

#18 @ethitter
6 years ago

  • Cc erick@… added

#19 in reply to: ↑ 9 @netweb
6 years ago

Replying to MZAWeb:

2) I need to change the importer in includes/admin/converters/bbPress1.php, but I don't have a clue how. Need to research how it works. (Never played with the importer)

Attached 1925.bbPress1.php.diff which removes that entire field mapping as we already map to post_modified a couple of lines up...

This would also need to be removed from all importers (phpBB, vBulletin etc) and is implemented identically in each importer, quick and easy once we are ready to commit this ticket.

#20 follow-up: @thebrandonallen
6 years ago

  • Keywords has-patch added

Hi! it's the bugist formally known as cnorris23. I'm including a more comprehensive patch. The current patch doesn't work because wp_insert_post(), and by extension, wp_update_post(), doesn't allow you to pass your own post_modified(_gmt) times. In order to do this, I've hooked into wp_insert_post_data, to essentially reset the post_modified(_gmt) times back to the times in the current post object. Also, at this time, since a true post edit time was mentioned early in the ticket, a _bbp_last_edit_time_gmt post meta key will be added with what should have been the new post_modified_gmt time. The other thing this patch does, is retain, and continue to add/update the _bbp_last_active_time for some back compat.

Things to note. Please do not run this on a production server, with production data. It won't break things per se, but your topics/forums definitely won't be properly sorted. This leads me to my final note. I have patches for both tools.php and the importers. The tools.php can use a little more work and cleanup, and I need to write a few queries to run during the upgrade routine. Since the queries being run in tools.php are little beasts, the same will go for the upgrade routine, I want to see how this patch is going to land before I spend too much time taming.

@thebrandonallen
6 years ago

Read and write post_modified. No database upgrading.

#21 @johnjamesjacoby
6 years ago

  • Milestone changed from 2.5 to 2.6

This patch is a great start. Any thoughts on what a database upgrade routine would look like?

#22 @johnjamesjacoby
6 years ago

  • Component changed from General to Performance Improvements
  • Keywords early added; dev-feedback removed

#23 @johnjamesjacoby
6 years ago

  • Summary changed from Performance: Optimize _bbp_last_active_time to use an indexed field for very large deployments to Optimize `_bbp_last_active_time` to use `post_modified` instead

#24 in reply to: ↑ 2 @netweb
6 years ago

Replying to johnjamesjacoby:

... maybe, try a subquery using the ID and post_parent indexes, but that only works if we can guarantee that newer replies will always have a higher ID. This *should* be the case, but it's hard to know for sure given the way the importer works, and how other forum software stores its data.

For the importers this 'should' always be the case. The way the current importer works is it will import all the forums, then topics, then replies so in theory it would be a rare case but not foolproof.

In theory the above would also mitigate against how 'other forum software' stores it's data as we would only be importing the data based on the way the current importer imports it's data.

What does cause an issue is the built in WordPress WXR Export/Import, it includes the current post_id during export and upon import if that post_id is not used it will use that existing post_id to assign to the freshly imported post.

Test Scenario:

  • Import an existing forum into my WP install bbPress/Import test site
  • As this site has done a couple of imports, currently the post ID increment in wp_posts is ~400,000 and in wp_postmeta 2,000,000
  • When I import a new forum, topic or reply it will get the next post_id available 400,001, 400,002
  • When I export those using WP's WXR Export the post_id is included in the export 400,001, 400,002
  • During the WXR import if the post_id in the target installation is available it will be used eg 400,001, 400,002 etc, etc
  • When new posts, pages, forums, topics are created they will be assigned post_id 2, 3, 4.... right up to the existing post_id's 400,001, 400,002 and then continue from the next available post_id.

Coming shortly, testing notes from patches attached to this ticket.

#25 @netweb
6 years ago

Will update my testing notes and fresh patches tomorrow with a clear head...

Anything I write at the moment is more confusing than helpful.

@netweb
6 years ago

#26 @netweb
6 years ago

Some data from each patch.

http://sqlfiddle.com/#!2/7898a/1 1925-mzaweb-wp_posts.sql
1925-mzaweb.png

SELECT * FROM wp_posts WHERE ID = 90581 OR post_parent = 90581

http://sqlfiddle.com/#!2/e6a05/1 1925-thebrandonallen-wp_posts.sql
1925-thebrandonallen.png

SELECT * FROM wp_posts WHERE ID = 42628 OR post_parent = 42628
Last edited 6 years ago by netweb (previous) (diff)

#27 @netweb
6 years ago

Oops, forgot wp_postmeta data in the above:

  • 1925-thebrandonallen wp_postmeta
42628 _bbp_last_active_time 2013-12-08 21:06:09
42628 _bbp_last_edit_time_gmt 2013-12-08 10:09:36
  • 1925-mzaweb wp_postmeta
90581_bbp_last_active_time2013-12-08 19:44:30

#28 follow-up: @netweb
6 years ago

1925-MZAWeb

  • This does 95% of what is needed and the only issue I see is when you edit a topic the topic freshness is set to the edited topic time.
  • This kind of puts us back where we started from in that we have used the post_date, post_date_gmt, post_modified & post_modified_gmt but if we were to use a wp_postmeta field to keep the topic freshness accurate we are then joining the wp_posts & wp_postmeta again.

Could we have the topic freshness read from post_modified if there are no revisions to the topic, if there are revsions and replies then freshness is calculated from the post_modified field of the last reply?


1925-thebrandonallen

  • All the revision times (post_date, post_date_gmt, post_modified & post_modified_gmt) are set to the latest topic freshness timestamp. As we want to keep the topic (and reply) edited revisions these timestamps need to remain accurate.
  • Both functions bbp_fix_post_modified & bbp_update_post_modified_helper are manipulating post_modified & post_modified_gmt fields with checks to only update forum or topic post types (I looked but could not find a way to stop this occurring to the revisions)
  • As _bbp_last_edit_time_gmt is the only place (currently based on the above) the true last edit time of a topic there is no way to recalculate this time as a Repair Tool in includes/admin/tools.php if things get out of sync.

Can the functions bbp_fix_post_modified & bbp_update_post_modified_helper be modified so that all the revisions don't end up setting all the timestamps to the most recent topic freshness?

#29 in reply to: ↑ 20 @netweb
6 years ago

Replying to thebrandonallen:

Also, at this time, since a true post edit time was mentioned early in the ticket, a _bbp_last_edit_time_gmt post meta key will be added with what should have been the new post_modified_gmt time.

This is not needed as the value is stored in the post_date of the latest post revision of the topic/reply, in this case topic post ID 90581's latest revision is post id 90610.

#30 @netweb
6 years ago

I added both SQL files as SQLFiddle started being weird :/

#31 @netweb
6 years ago

That's it for now, head is swimming in data... Any thoughts, comments or corrections to my understanding of the logic here would be greatly appreciated.

#32 @netweb
6 years ago

One more note, if you use the 'Recent Topics' widget with the view 'Popular' we will again be joining the wp_posts & wp_postmeta tables as this is calculated via wp_postmeta._bbp_reply_count.

#33 in reply to: ↑ 28 ; follow-up: @thebrandonallen
6 years ago

Replying to netweb:

  • Good catch. It didn't do that at one point. Took some tracking down, but I found the issue. I'll get patch up tonight hopefully. Gotta remove some debug code.
  • bbp_update_post_modified_helper was stamping out the revision dates. Just needed to do a stricter post_type check
  • This is true. It's a side effect of trying to use post_modified. I thought about splitting post_modified and post_modified_gmt making one the freshness and leaving the other the as the true modified time, but that would have been incredibly confusing.
  • This is correct as well. However, my goal was to keep in line with the current system. The way it's done now is we create a last_active time when a reply/topic is created, then propagate that time up the chain to the parent topic/forum. Solely using wp_update_post would work, but we wouldn't be able to specify a specific time as we do now, and we run the risk of having times that are out of sync, and not representative of the relative child element. Here's an example. With our current system (preserved in my patch) a reply is created at 2013-12-09 10:10:10, and the topic/forum last active meta values will be the same, 2013-12-09 10:10:10. Using wp_update_post could very easily result in that same reply resulting in a topic last active of 2013-12-09 10:10:11, and forum last active of 2013-12-09 10:10:12. On a busy forum on a shared host (a very likely scenario), the disparity could be much worse. For most, this difference will be negligible, but I figured it was better to maintain the consistent outcome we have currently. Obviously, wp_update_post would greatly simplifies the code.

Replying to netweb:

This is not needed as the value is stored in the post_date of the latest post revision of the topic/reply, in this case topic post ID 90581's latest revision is post id 90610.

I originally thought the same thing, but then it struck me that revisions are optional. You can disable them in bbPress, which removes the option to save a revision in the topic/forum edit forms, or they can be disabled system wide via a WP filter or constant. As such, we can't depend on a revision to be available to use, hence, _bbp_last_edit_time_gmt.

Thanks for reviewing the ticket!

#34 in reply to: ↑ 33 @netweb
6 years ago

Replying to thebrandonallen:

  • This is correct as well. However, my goal was to keep in line with the current system. The way it's done now is we create a last_active time when a reply/topic is created, then propagate that time up the chain to the parent topic/forum. Solely using wp_update_post would work, but we wouldn't be able to specify a specific time as we do now, and we run the risk of having times that are out of sync, and not representative of the relative child element. Here's an example. With our current system (preserved in my patch) a reply is created at 2013-12-09 10:10:10, and the topic/forum last active meta values will be the same, 2013-12-09 10:10:10. Using wp_update_post could very easily result in that same reply resulting in a topic last active of 2013-12-09 10:10:11, and forum last active of 2013-12-09 10:10:12. On a busy forum on a shared host (a very likely scenario), the disparity could be much worse. For most, this difference will be negligible, but I figured it was better to maintain the consistent outcome we have currently. Obviously, wp_update_post would greatly simplifies the code.

We don't need to apply a specific time as WordPress handles this natively:

  • A new post is created using wp_insert_post and the current timestamp is set in post_date, post_date_gmt, post_modified & post_modified_gmt
  • An updated/edited post uses wp_update_post and the original post_date & post_date_gmt times are kept with post_modified & post_modified_gmt updated with the current timestamp.

I'll come back to the issue of possible last active meta conflicts _bbp_last_active_time below.

  • bbp_update_post_modified_helper was stamping out the revision dates. Just needed to do a stricter post_type check

Yes, I thought that was the case.

I originally thought the same thing, but then it struck me that revisions are optional. You can disable them in bbPress, which removes the option to save a revision in the topic/forum edit forms, or they can be disabled system wide via a WP filter or constant. As such, we can't depend on a revision to be available to use, hence, _bbp_last_edit_time_gmt.

We are still good here and don't need _bbp_last_edit_time_gmt

  • WordPress Revisions disable define('WP_POST_REVISIONS', false );
  • bbPress Revisions disable _bbp_allow_revisions = 0

In both these cases post_modified & post_modified_gmt dates are updated to the correct time of the last topic edit.

Coming back to 'Topic Freshness' / _bbp_last_active_time and function bbp_get_topic_last_active_time (/includes/topics/template.php#L1791)

  • template.php

     
    17911789        function bbp_get_topic_last_active_time( $topic_id = 0 ) {
    17921790                $topic_id = bbp_get_topic_id( $topic_id );
    17931791
    1794                 // Try to get the most accurate freshness time possible
    1795                 $last_active = get_post_meta( $topic_id, '_bbp_last_active_time', true );
    1796                 if ( empty( $last_active ) ) {
    1797                         $reply_id = bbp_get_topic_last_reply_id( $topic_id );
    1798                         if ( !empty( $reply_id ) ) {
    1799                                 $last_active = get_post_field( 'post_date', $reply_id );
    1800                         } else {
    1801                                 $last_active = get_post_field( 'post_date', $topic_id );
    1802                         }
     1792                // Check the topic for the last reply id
     1793                $reply_id = bbp_get_topic_last_reply_id( $topic_id );
     1794
     1795                // If we have a reply use that timestamp, if not use the topic timestamp
     1796                if ( !empty( $reply_id ) ) {
     1797                        $last_active = get_post_field( 'post_date', $reply_id );
     1798                } else {
     1799                        $last_active = get_post_field( 'post_date', $topic_id );
    18031800                }
    18041801
    18051802                $last_active = !empty( $last_active ) ? bbp_get_time_since( bbp_convert_date( $last_active ) ) : '';

Rather than go getting the _bbp_last_active_time we can use the native post time stamps as they are accurate and just use the existing method to calculate the freshness based on if the topic has a reply use the last reply post_date, if no replies use the topic post_date

This works for all combinations of revisions...

Patch incoming....

Last edited 6 years ago by netweb (previous) (diff)

@netweb
6 years ago

#35 @netweb
6 years ago

  • Keywords needs-testing added

In 1925.04.diff

_bbp_last_active_time is kept for back compat, most likely will be found in use with custom views.

I am messing about with the tools.php at the moment trying to work out if we need a database upgrade routine is needed or not and I will add the converters to this also.

I will also run a couple of SQL queries as per @MZAWeb's previous tests.

p.s. There are a few extra PHPDoc updates in the includes/forums/functions.php section.

@netweb
6 years ago

#36 @netweb
6 years ago

  • Summary changed from Optimize `_bbp_last_active_time` to use `post_modified` instead to Optimize `_bbp_last_active_time` to use `post_date` instead

Boom! Patch no longer uses post_modified and now uses post_date

In 1925.05.diff we now use post_date and let WordPress handle 'all the things'

  • Topics and Replies are ordered by post_date
  • Stickies not in original topic query ordered by post_date order ASC
  • Forum, Topic & Reply revision dates are handled by default WordPress revision handling

#37 @netweb
6 years ago

DB Size:
forums 15
topics ~20,000
replies ~30,000

1. The forum in this query has only ~1,000 topics
2. I'd hardly call my local MySQL optimized either, out of the box config.
3. Query times are the average of 20 runs. First couple of runs ignored as to get MySQL "hot".

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM   wp_posts
       INNER JOIN wp_postmeta
               ON ( wp_posts.ID = wp_postmeta.post_id )
WHERE  1 = 1
   AND wp_posts.post_parent = 42642
   AND wp_posts.post_type = 'topic'
   AND ( wp_posts.post_status = 'publish'
          OR wp_posts.post_status = 'closed'
          OR wp_posts.post_status = 'private' )
   AND ( wp_postmeta.meta_key = '_bbp_last_active_time' )
GROUP  BY wp_posts.ID
ORDER  BY wp_postmeta.meta_value DESC
LIMIT  0, 15

Average query time: 374.8ms

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM   wp_posts
WHERE  1 = 1
   AND wp_posts.post_parent = 42642
   AND wp_posts.post_type = 'topic'
   AND ( wp_posts.post_status = 'publish'
          OR wp_posts.post_status = 'closed'
          OR wp_posts.post_status = 'private' )
 GROUP  BY wp_posts.ID
ORDER  BY wp_posts.post_date DESC
LIMIT  0, 15

Average query time: 12.1ms

#38 follow-up: @vibol
6 years ago

Hi, thank you all for putting time and energy into this issue. Please let me know when I should spend time to build a test environment and run these patches against them. Also, it just occurred to me that I never uploaded my adapted patch from long ago. I have it running in production now and it's been working for many months. I made some changes in there that may still be applicable for the Tools section. However, my patch is against 2.2.4 and last_modified. See attached.

#39 in reply to: ↑ 38 @netweb
6 years ago

Replying to vibol:

Hi, thank you all for putting time and energy into this issue. Please let me know when I should spend time to build a test environment and run these patches against them.

Now would be great and extremely helpful to get another set of eyes on the data :)

I am still digging through the logic and confirming 'existing topics' order but initial thoughts are everything is fine and no database upgrade routine will be needed.

The quickest way I see would be to make a copy of your current database and setup a fresh install, apply this patch and then compare both performance and the logic to make sure all the things are ordered correctly and there are no issues with topic or reply edits and revisions.

Replying to vibol:

Also, it just occurred to me that I never uploaded my adapted patch from long ago. I have it running in production now and it's been working for many months. I made some changes in there that may still be applicable for the Tools section. However, my patch is against 2.2.4 and last_modified.

At a quick glance the patch is very similar rather than use post_modified we apply the same logic but use post_date which would be very similar performance.

Any calls to update post_modified manually are no longer required (nor did they work unless a direct SQL write made the modifications) and as this is part of WordPress CORE we now just let WordPress' wp_insert_post and wp_update_post do it's thing.

#40 @netweb
6 years ago

Related: This patch may also address the freshness issue per #2414

#41 follow-up: @thebrandonallen
6 years ago

Topics are reporting the correct freshness, but sorting by topic freshness (the default method) does not work. Topics are sorted by the date they were originally posted, not by the time of the most recent reply.

Last edited 6 years ago by thebrandonallen (previous) (diff)

#42 @johnjamesjacoby
6 years ago

In 5221:

In bbp_update_reply(), send the reply post_date - rather than current_time() - into the update walker. Fixes possibility of inconsistent times between reply post_date and forum/topic last active times. Props netweb. See #1925.

#43 @johnjamesjacoby
6 years ago

In 5222:

In bbp_update_topic(), send the topic post_date - rather than current_time() - into the update walker. Fixes possibility of inconsistent times between topic post_date and forum last active times. Props netweb. See #1925.

#44 @johnjamesjacoby
6 years ago

In 5223:

In bbp_update_topic_walker() pass $last_active_time into bbp_update_forum() to avoid expensive recalculation of the forum's last active time. Props netweb. See #1925.

#45 follow-up: @thebrandonallen
6 years ago

Thanks johnjamesjacoby. This fixes the problem with inconsistent times I found and mentioned earlier. Just waiting to see what netweb comes up with to fix the sorting by last active.

#46 @johnjamesjacoby
6 years ago

Above patch switches back to post_modified to avoid issues with ordering forums/topics/replies by their original creation date, which should still be possible to do.

@johnjamesjacoby
6 years ago

Includes _update_ functions, removes phpdoc changes from previous patches

#47 @thebrandonallen
6 years ago

This brings us back to my patch. wp_update_post does not allow you to pass a custom time to post_modified, and post_modified_gmt.
http://core.trac.wordpress.org/browser/trunk/src/wp-includes/post.php#L2819

#48 @thebrandonallen
6 years ago

I've just realized there is a way to do it using wp_update_post. This should get rid of the need for the bbp_update_(forum/topic)_post_modified() and bbp_update_post_modified_helper() functions. It should also fix the issue of revision times being set to last active time. Will get my thoughts in patch form later this evening.

#49 in reply to: ↑ 41 @netweb
6 years ago

Awesome 1925.patch patch :)

1925.2.patch is a refresh with changes:

  • Less the bbp_update_topic() r5222 commit
  • Forum freshness bbp_get_forum_last_active_time fixed

Just one last issue to fix as $topic_id post_modified currently gets refreshed when a topic is edited.

Gone looking at WP's revisions code to see if we can check for a revision before firing wp_update_post in bbp_update_forum_last_active_time

Last edited 6 years ago by netweb (previous) (diff)

@netweb
6 years ago

Fixed Copy Pasta Error ;)

@netweb
6 years ago

Updated converters:

#50 @netweb
6 years ago

In 1925.converters.patch updates all the bbPress importers:

  • Removes _bbp_last_active_time topic date import from all importers
  • Remaps topic date field mapping using to post_modified (Vanilla, vBulletin3, vBulletin & phpBB)

#51 follow-up: @thebrandonallen
6 years ago

The above patch 1925.post_modified.01.diff makes some improvements to my previous patch.

  • I've updated to using wp_update_post as this seems to be the desired approach. It's definitely easier to maintain.
  • bbp_fix_post_modified sticks around, for two reasons. 1) In order for the custom post_modified(_gmt) times that we pass to wp_update_post to actually be used, we need to make sure the are set. 2) this is a good place to set the _bbp_last_edit_time_gmt. This allows us to have an edit time that can function as the post_modified time that we are losing. Plus, it's needed to fix revision times. I have updated the logic, so we are no longer potentially making calls the db.
  • Introduce bbp_fix_revision_times. Revisions use the post_modified(_gmt) from the post they are revising to set the post date times to set the corresponding post_date(_gmt) fields. Since we're using post_modified for fresheness, this is throwing off the revision times. We pull _bbp_last_edit_time_gmt, and use it as a starting point to rebuild the proper date values.
  • Right before I posted this, I realized that replies were creating post revisions, since we're using wp_update_post now. I've added in the revisions suspension logic from the edit topic handler to stop this.

@thebrandonallen
6 years ago

post_modified - with wp_update_post, allows us to pass our own post_modified(_gmt) times, fixes issue with revisions

#52 in reply to: ↑ 51 @johnjamesjacoby
6 years ago

  • Keywords commit added; has-patch needs-testing removed
  • Type changed from defect to enhancement

Thanks so much for that. It seems to work really well. I refreshed the patch to add a revision check for forums, just in case they ever do get revisions.

This is ready to commit whenever we have a database upgrade routine.

#53 follow-up: @johnjamesjacoby
6 years ago

We should make sure that reply revision date meta works the same as topics do, to keep everything consistent.

#54 in reply to: ↑ 53 @thebrandonallen
6 years ago

Replying to johnjamesjacoby:

We should make sure that reply revision date meta works the same as topics do, to keep everything consistent.

When I figured out the issue with topics, I double checked replies, and there didn't seem to be any issues, but more eyes would be good. Mine were tired.

#55 @netweb
6 years ago

Testing 1925.3.patch now...

Looks good so far with all the

  • 'freshness' times accurate for template notices/lists including new topics/replies added.
  • Topic edited content and time is good
  • Reply edited content and time is good
  • Move a topic via front/back end is good
  • Split topic times look good
  • Wishing we had PHPUnit tests

Issues:

  • Reply Revisions
    • An edited reply works as expected, it does not bump the freshness and the edited content IS correct though:
    • Reply post revisions are not created as posts in wp_posts
    • Reply revisions log _bbp_revision_log is not created in wp_postmeta
    • All the functions for the above are there, it just doesn't happen, needs more investigation.
  • Merge Topic (Related #2329)
    • Merge 'TopicB 2:00pm' topic into 'TopicA 1:00pm'
    • 'TopicA' freshness is now 2:00pm
    • Merge 'TopicC 3:00pm' topic into 'TopicA 1:00pm'
    • 'TopicA 1:00pm' topic freshness remains at 2:00pm
    • This is a wider topic merge issue than this ticket specifically I think.

Importers

  • As per the attached patch above where I removed _bbp_last_active_time from each importer as this should only be the last known edit/update time stamp of the topic id. Just need to think about the logic of each forum software a bit before writing a patch.

Tools // Repair topic revisions last edit time _bbp_last_edit_time_gmt
I think this SQL is good, another set of eyes to confirm

INSERT INTO `$wpdb->postmeta` (`post_id`, `meta_key`, `meta_value`)
SELECT topic.post_parent, '_bbp_last_edit_time_gmt ', topic.post_date_gmt
FROM wp_posts AS topic
WHERE topic.post_parent =90702
ORDER BY topic.ID DESC
LIMIT 1

Database Upgrade

  • ToDO - Still thinking about this ;)

Performance Average query time: 13.2ms

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM wp_posts
WHERE 1 =1
AND wp_posts.post_parent =42642
AND wp_posts.post_type =  'topic'
AND (
wp_posts.post_status =  'publish'
OR wp_posts.post_status =  'closed'
OR wp_posts.post_status =  'private'
OR wp_posts.post_status =  'hidden'
)
ORDER BY wp_posts.post_modified DESC 
LIMIT 0 , 15

#56 follow-up: @netweb
6 years ago

BLOCKER: Current itteration breaks ALL the importers

When importing we use wp_insert_post to insert each forum, topic, reply post resulting in:

WordPress database error: [Incorrect datetime value: '' for column 'post_date_gmt' at row 1]

We need to remove the two filters when doing an import and them back once finished.
remove_filter( 'wp_insert_post_data', 'bbp_fix_post_modified');
remove_filter( 'wp_insert_post_data', 'bbp_fix_revision_times');

#57 in reply to: ↑ 56 @thebrandonallen
6 years ago

Replying to netweb:

BLOCKER: Current itteration breaks ALL the importers

When importing we use wp_insert_post to insert each forum, topic, reply post resulting in:

WordPress database error: [Incorrect datetime value: '' for column 'post_date_gmt' at row 1]

We need to remove the two filters when doing an import and them back once finished.
remove_filter( 'wp_insert_post_data', 'bbp_fix_post_modified');
remove_filter( 'wp_insert_post_data', 'bbp_fix_revision_times');

I wasn't able to reproduce this. In my testing, the imported forum/topic/reply never made it far enough through the 'if' logic to even have their dates adjusted. bbp_fix_revision_times is the only function the messes with post_date_gmt, but, since we don't insert revisions, the logic that messes with post_date_gmt never runs. I only have bbPress 1 data to run import tests, but I was never able to get the db error to occur.

#58 @mordauk
6 years ago

  • Cc pippin@… added

#59 follow-up: @netweb
6 years ago

  • Keywords commit removed

Taking 1925.3.patch for another spin...

The importer now seems to work with this, not sure why it didn't work previously.

Here is a copy of my standard phpBB SQL Database ntwb_phpbbv3.zip (66kb) if someone else could test this to eliminate my setups out of the equation that would be grand.

bbPress Importer Settings: Select Platform phpBB | Database Name ntwb_phpbbv3 | Table Prefix phpbb_
(The remaining settings are your own primary database ip address, username, password etc)


Creating a forum in the back end fails, technically it is created, but admin doesn't come back, you are left with a blank white screen.

Fatal error: Maximum function nesting level of '100' reached, aborting! in C:\xampp\htdocs\wp-includes\functions.php on line 2837


Replying to johnjamesjacoby:

post_modified *almost* makes sense, but we should be fully aware of what WordPress core uses that for before hijacking it. If it's used for comparing revisions, we're out of luck; if it's not used at all, we're at risk of it being deprecated and disappearing completely (like post_category).

Has anyone tested post/page revisions with this? I have now :(

Blocker: 1925.3.patch breaks post revisions

Repro:

  • Deactivate bbPress
  • Create a WordPress post and publish
  • Edit post and update
  • Make another edit and see post revisions below post body
  • Activate bbPress
  • Edit the same post again, no new revisions are created
  • The post content is updated reflecting the edit changes
  • No new revision post is created in wp_posts
  • Deactivate bbPress, edit post and a new revision is displayed
  • Check the wp_posts table and a new revision now exists

Replying to netweb:

Issues:

  • Reply Revisions
    • An edited reply works as expected, it does not bump the freshness and the edited content IS correct though:
    • Reply post revisions are not created as posts in wp_posts
    • Reply revisions log _bbp_revision_log is not created in wp_postmeta
    • All the functions for the above are there, it just doesn't happen, needs more investigation.

Reply Revisions work as expected using /trunk and do NOT work with 1925.3.patch

This ticket was mentioned in IRC in #bbpress-dev by netweb. View the logs.


6 years ago

#61 in reply to: ↑ 45 @netweb
6 years ago

Replying to thebrandonallen:

Just waiting to see what netweb comes up with to fix the sorting by last active.

I found a solution for this through changing the ORDER BY in bbp_has_topics() and performance is ~4x

Current: Query took 0.0675 sec (897 topics and 1,836 replies)

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM wp_posts 
INNER JOIN wp_postmeta
ON (wp_posts.ID = wp_postmeta.post_id)
WHERE 1=1 
AND wp_posts.post_parent = 96479 
AND wp_posts.post_type = 'topic'
AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'closed'
OR wp_posts.post_status = 'private'
OR wp_posts.post_status = 'hidden')
AND (wp_postmeta.meta_key = '_bbp_last_active_time' )
GROUP BY wp_posts.ID
ORDER BY wp_postmeta.meta_value DESC
LIMIT 0, 15

Solution: Query took 0.0173 sec (897 topics and 1,836 replies)

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM wp_posts 
WHERE 1=1 
AND wp_posts.post_parent = 96479 
AND wp_posts.post_type = 'topic'
AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'closed'
OR wp_posts.post_status = 'private'
OR wp_posts.post_status = 'hidden') 
ORDER BY COALESCE( (
SELECT MAX( post_date )
FROM wp_posts r1
WHERE r1.post_parent = wp_posts.id ) , wp_posts.post_date ) DESC
LIMIT 0, 15

The function uses /Plugin_API/Filter_Reference/posts_orderby as this type of custom SQL cannot be parsed directly by WP_Query's 'ORDERBY' parameter

This was tested alongside 1925.05.diff.

The function as it stands changes the "ORDER BY" for EVERY WP_Query so we would need to use apply_filter and remove_filter before/after bbp_has_topics $bbp->topic_query

function bbp_topic_freshness_override($orderby_statement) {

        $orderby_statement = "COALESCE( ( SELECT MAX( post_date ) FROM wp_posts r1 WHERE r1.post_parent = wp_posts.id ) , wp_posts.post_date ) DESC";
        return $orderby_statement;
}
add_filter('posts_orderby', 'bbp_topic_freshness_override');

It has some larger ramifications for bbp_has_topics() though as the custom 'ORDER BY' can not be changed by end users eg. bbp_has_topics( array( 'orderby' => 'title', 'order' => 'ASC' ) )

To keep current back compat these would need to be moved into a new function using /Plugin_API/Filter_Reference/posts_clauses (including 'posts_orderby').

eg. (idea, not tested ;))

function bbp_topic_freshness_override($orderby_statement) {

        if( $bbp->topic_query['orderby'] == 'date' ) {
                $orderby_statement = "COALESCE( ( SELECT MAX( post_date ) FROM wp_posts r1 WHERE r1.post_parent = wp_posts.id ) , wp_posts.post_date ) DESC";
        }

        if( $bbp->sticky_query ) {
                $orderby_statement = "wp_posts.post_date ASC";
        }

        if( $bbp->topic_query['orderby'] == 'title' ) {
                $orderby_statement = "wp_posts.post_title DESC";
        }
        if( $bbp->topic_query['orderby'] == 'etc' ) {
                $orderby_statement = "etc etc etc";
        }       
                
        return $orderby_statement;
}
apply__filter('posts_orderby', 'bbp_topic_freshness_override');

It has some benefits also that a similar function could be used for forums topic freshness also.

I haven't taken to rewrite bbp_has_topics() for the above 'proof on concept' but everyone's thoughts on this method would be greatly appreciated :)

#62 in reply to: ↑ 59 @thebrandonallen
6 years ago

Replying to netweb:

The importer now seems to work with this, not sure why it didn't work previously.

Here is a copy of my standard phpBB SQL Database ntwb_phpbbv3.zip (66kb) if someone else could test this to eliminate my setups out of the equation that would be grand.

Thanks for the test data! I gave it a go, and had no problems as well.


Creating a forum in the back end fails, technically it is created, but admin doesn't come back, you are left with a blank white screen.

Fatal error: Maximum function nesting level of '100' reached, aborting! in C:\xampp\htdocs\wp-includes\functions.php on line 2837

A fun little infinite loop was being created between bbp_update_forum_last_active_time() and BBP_Forums_Admin::attributes_metabox_save().


Has anyone tested post/page revisions with this? I have now :(

Blocker: 1925.3.patch breaks post revisions

I wasn't able to reproduce this. Whether bbPress was disabled, or active and patched/unpactched for post_modified, post revisions were properly created.


Replying to netweb:

Issues:

  • Reply Revisions
    • An edited reply works as expected, it does not bump the freshness and the edited content IS correct though:
    • Reply post revisions are not created as posts in wp_posts
    • Reply revisions log _bbp_revision_log is not created in wp_postmeta
    • All the functions for the above are there, it just doesn't happen, needs more investigation.

Reply Revisions work as expected using /trunk and do NOT work with 1925.3.patch

I wasn't able to reproduce this either. Post revisions were created in the wp_posts table, and _bbp_revision_log was correctly updated.

#63 follow-up: @johnjamesjacoby
6 years ago

To be clear, we cannot use post_date and should be using post_modified. post_date is reserved for the creation date, which is still important to bbPress and WordPress.

#64 @thebrandonallen
6 years ago

  • Summary changed from Optimize `_bbp_last_active_time` to use `post_date` instead to Optimize `_bbp_last_active_time` to use `post_modified` instead

Attached patch brings a few changes/additions. Most notably, is the fix for the infinite loop netweb stumbled upon. The other changes are just logic improvements and PHPdoc cleanup. I've also included both the importer updates, the tools.php updates, and a possible upgrade routine in this patch. This should, hopefully, make it easier to make sure all the necessary code is there during testing.

As far as the upgrade routine goes. I've intentionally not included the required db bump to make the upgrader run automatically. Wanted to make it just a little bit harder for people to add this to a production server. It definitely needs some stress testing. The queries shouldn't be too taxing, but my test install only has about ~5000 combined forums/topics/replies.

#65 in reply to: ↑ 63 @netweb
6 years ago

Replying to johnjamesjacoby:

To be clear, we cannot use post_date and should be using post_modified. post_date is reserved for the creation date, which is still important to bbPress and WordPress.

It looks like I have not described myself well in my comments above, so here goes, per the original ticket:

Replying to vibol:

JJJ: Meta queries are always going to be a slow like this. There isn't an available index we can use here to avoid the meta query. We *could* hack in repurposing post_date post_modified as the freshness time..

I understand the need to use post_modified for 'THIS' solution with the proposed ideas and patches by @MZAWeb & @thebrandonallen

The patch I added with the comment 'Boom' (1925.05.diff) was because I had thought I had all the parts of the puzzle solved and there would be no need need for bbPress to use _bbp_last_active_time or the need for bbPress modify any of the four post date fields in wp_posts, the problem was I did not have all the parts to this idea and topics were sorted by topic creation date rather than 'freshness'.

Per my latest #comment:61 I believe I now have 'all the parts' for this solution to work perfectly. It is extremely similar this: (Note: The repair tools currently use the 'highest reply ID' to calculate the last reply.)

Replying to johnjamesjacoby:

We could also, maybe, try a subquery using the ID and post_parent indexes, but that only works if we can guarantee that newer replies will always have a higher ID. This *should* be the case, but it's hard to know for sure given the way the importer works, and how other forum software stores its data.

The key to this was the SQL COALESCE query, it acts kind of like an inner join of the wp_posts table to Return the first non-NULL argument (MySQL Docs). This lets us for example get a list of topics and include the last reply id and/or date in that single query.

This allows us to include MAX ( 'post_date') / MAX ('ID') / MAX( 'reply'.'ID' ) type queries in the current bbp_has_forums / bbp_has_topics queries. These new queries are near identical to the current repair tool queries that are used to rebuild these fields in wp_postmeta. As we can actually include them in bbPress' primary queries we gain significant performance improvements and no longer need to store this data in a wp_postmeta.bbp_last_active_* field (or wp_posts.post_modified) field.

My 'proposed solution' enables bbPress to no longer need to store, use or query these wp_postmeta fields:
(Each of following can be included in existing bbp_has_forums, bbp_has_topics queries as a single $WPDB query of the wp_posts table.)

  • _bbp_last_active_time
  • _bbp_last_active_id
  • _bbp_last_reply_id
  • _bbp_last_topic_id

Possibly also: (I haven't gone completely down this rabbit hole)

  • _bbp_forum_id
  • _bbp_topic_id
  • _bbp_reply_id
  • _bbp_reply_to
  • _bbp_status

The following will need to be retained in wp_postmeta:

  • _bbp_anonymous_email, _bbp_anonymous_name & _bbp_anonymous_website
  • _bbp_author_ip
  • _bbp_forum_subforum_count
  • _bbp_forum_type
  • _bbp_group_ids
  • _bbp_reply_count ,_bbp_reply_count_hidden, _bbp_total_reply_count
  • _bbp_topic_count, _bbp_topic_count_hidden, _bbp_total_topic_count
  • _bbp_voice_count
  • _bbp_revision_log

The following repair tools would no longer be required:
(bbPress would be using WP's native and original untouched timestamp and date wp_posts fields)

  • Recalculate the parent topic for each post
  • Recalculate the parent forum for each post
  • Recalculate last activity in each topic and forum

There is a fair amount of code and testing required for all of the improvements above so I would think if we removed _bbp_last_active_time and _bbp_last_active_id for starters we can then go from there.

In summary, is what I have outlined above good idea? I believe it is.

  • Comparable performance to the post_modified solution.
  • We are not modifying WordPress Core table database field for our own purpose
  • Future proof if WordPress Core 'did' decide to do 'something' in the future with post_modified
  • For starters two less bbPress wp_postmeta fields

Am I bonkers? crazy? nuts? If I'm not I'll upload a patch, if I am lets go with the post_modified solution :)

#66 @thebrandonallen
6 years ago

You're not bonkers if it works ;), but I think the only way to see if it works, is to see the code, and see it in action.

@netweb
6 years ago

#67 follow-up: @netweb
6 years ago

In 1925.alt.01.diff

'Proof of Concept' patch ;)

Some things to note:

  • function bbp_topic_freshness_override is pretty hacky and does what is needed for Proof of Concept
  • Only works for a 'single forum' list of topics sorted by latest reply (bbp_has_topics)
    • (Latest replies wp_posts.post_date or the topics wp_posts.post_date if the topic has no replies)
  • Other conditions will need some different SQL queries eg search, widgets etc
  • Still a bit to do here to test all the things and implement the same on the 'forum' side for 'last active topic'
  • Support for legacy uses of custom forum, topic or reply 'orderby' and 'order' ASC/DESC
    • eg bbp_has_topics( array( 'orderby' => 'title', 'order' => 'ASC' ) )

#68 @sc0ttkclark
6 years ago

  • Cc scott@… added

I'm seeing major slow downs on two specific areas:

# Time: 140212 17:48:21
# Query_time: 2.377407  Lock_time: 0.000123 Rows_sent: 15  Rows_examined: 494974
SET timestamp=1392227301;
SELECT SQL_CALC_FOUND_ROWS  wp_2_posts.ID FROM wp_2_posts  INNER JOIN wp_2_postmeta ON (wp_2_posts.ID = wp_2_postmeta.post_id) WHERE 1=1  AND wp_2_posts.post_parent = 3846935  AND wp_2_posts.post_type = 'topic' AND (wp_2_posts.post_status = 'publish' OR wp_2_posts.post_status = 'closed') AND (wp_2_postmeta.meta_key = '_bbp_last_active_time' ) GROUP BY wp_2_posts.ID ORDER BY wp_2_postmeta.meta_value DESC LIMIT 0, 15 /* In [/wp-content/plugins/bbpress/includes/topics/template.php:203] */;

And:

# Time: 140212 17:31:07
# Query_time: 8.857887  Lock_time: 0.005828 Rows_sent: 1  Rows_examined: 1603732
SET timestamp=1392226267;
SELECT COUNT(ID) FROM wp_2_posts WHERE post_parent IN ( 8811487,8811483,8811482,…,8717184,8708196 ) AND post_status = 'publish' AND post_type = 'reply'; /* In [/wp-content/plugins/bbpress/includes/forums/functions.php:1507] */;

This specific database has millions of topics/replies, with a pretty heavy meta table (bbPress meta stuff, nothing custom).

#69 @johnjamesjacoby
6 years ago

Thanks Scott for the additional data. This ticket is geared towards addressing those two issues specifically, amongst several others. Stephen's been doing a great job at experimenting here, and we'll continue to update with any progress that's made.

#70 @sc0ttkclark
6 years ago

Is there any specific patches submitted here that someone as desperate as me can look towards for immediate gains?

#71 @thebrandonallen
6 years ago

I my testing 1925.4.diff is stable, but it's vastly different than what netweb is currently exploring. You want do irreparable harm to your database, but it will mean the co-opting of the post_modified(_gmt) fields in for topics and forums. If netweb's approach gets the green light over the post_modified route, you can create/and run a query that resets your post_modified(_gmt) fields back to their original values if the data accuracy of those fields is important to you. I know you know this, but, please, please, pretty please test it before you go live :). If you decide to test it you'll want to set the upgrade routine to compare at a lower db version than what you currently have installed, but higher than the db version for the last upgrade routine. I intentionally made a barrier to entry on that one. Minor, but intentional.

netweb's 1925.alt.01.diff looks promising, but it, currently, only covers topic sorting.

#72 @sc0ttkclark
6 years ago

So I took care of a couple of the slow queries in a temporary way. The first ignores the reply counting for forums themselves (we don't care to show reply numbers). The second handles # of voices counting for a topic, in which we simply just want to get the current value from _bbp_voice_count and return that, instead of trying to calculate it over and over each time a new topic/reply is added.

function my_bbp_optimize_queries( $query ) {

	if ( false !== strpos( $query, 'SELECT COUNT(ID) FROM ' )
		 && false !== strpos( $query, ' ) AND post_status = \'publish\' AND post_type = \'reply\'' ) ) {
		$query = 'SELECT "0" AS `the_count`';
	}
	elseif ( false !== strpos( $query, 'SELECT COUNT( DISTINCT post_author ) FROM ' )
			 && false !== strpos( $query, ' AND post_status = \'publish\' AND post_type = \'reply\' ) OR ( ID = ' )
			 && false !== strpos( $query, ' AND post_type = \'topic\' )' ) ) {

		preg_match( '/ AND post_status = \'publish\' AND post_type = \'reply\' \) OR \( ID = (\d+) AND post_type = \'topic\' \)/', $query, $topic );

		$count = (int) get_post_meta( $topic[ 1 ], '_bbp_voice_count', true );

		if ( empty( $count ) ) {
			$count = 1;
		}

		$query = 'SELECT ' . $count . ' AS `the_count`';
	}

	return $query;

}
add_filter( 'query', 'my_bbp_optimize_queries' );

#73 @sc0ttkclark
6 years ago

FYI, my solution has some flaws for voice count, it could be better, but you can still see what I was after.

This ticket was mentioned in IRC in #bbpress-dev by netweb. View the logs.


6 years ago

#75 @netweb
6 years ago

I just bumped into Scott Reilly's (coffee2code) Last Contacted plugin and this uses a custom orderby coalesce statement.

The following two functions are a good reference of the proposed implementation of adding and removing the add_filter( 'posts_orderby'... filter conditionally

I'll take a closer look once we have some unit tests added to test this ticket.

This ticket was mentioned in IRC in #bbpress-dev by netweb. View the logs.


5 years ago

#77 @netweb
5 years ago

  • Milestone changed from 2.6 to 2.7

Bumping to 2.7 to get 2.6 out the door

This ticket was mentioned in Slack in #bbpress by thebrandonallen. View the logs.


5 years ago

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


4 years ago

#80 @alexander.rohmann
4 years ago

Hi there,

I've been following this issue for a while now, and understandably, I see this might take a bit more time. I'm working with a bbPress site with ~55K topics and ~275K replies. We started to feel the weight of this a few months ago, but now it's really noticable.

Out of all the patches here, is there one recommended for an interim fix? Something that is more in line with what is planned to be the official solution. Our forums are only getting bigger so I'm seriously considering patching it myself. It would be great to know where's the best place to start.

Thanks!

Last edited 4 years ago by alexander.rohmann (previous) (diff)

#81 in reply to: ↑ 67 @alexander.rohmann
4 years ago

Any other progress here? I'd be happy to help with any testing.

@netweb and @thebrandonallen, I've been doing some testing with these patches in a local environment working with data copied from our production site. Before any modifications, my average query time is right at 2s

Average query times:

1925.alt.01.diff - 950ms

1925.4.diff - 280ms

For the time being I'm going to setup some short lived page caching on my forum index, but I've got to make a call for production soon and I'm leaning towards biting the bullet and running the db migration for 1925.4.diff

Last edited 4 years ago by alexander.rohmann (previous) (diff)

#82 @thebrandonallen
4 years ago

I don't know that I can recommend either at the moment. While the results are impressive, after some discussion, I don't think we're going to take the approach in 1925.4.diff. The 1925.alt.01.diff is more proof-of-concept at the moment, but is likely the approach we'll be taking. Until fairly recently, we haven't had the framework to test patch like this. So, that being said, knowing it hasn't been thoroughly tested, if you wanted to use it, you would need to make sure you change all the references to wp_posts to match your actual table name. Even, better would be to make it a reference to wpdb::posts. For now, I would skip the patch, and use an opcache and/or object caching PHP extension, or some sort of page caching as you mentioned.

#83 @netweb
3 years ago

Via https://bbpress.org/forums/topic/bbpress-for-huge-forums-capable/page/2/#post-176909


I have a website that is 1 yo but already have 27 million posts and is only online because of the 1925.4.diff that bbpress staff/helpers provided here:

Even with the patch… I have some trouble with long threads using too much cpu… so I’m always closing threads because of that.

I also got in trouble with high cpu load when we have too many topics under the same forum… because of this problem.. from time to time I create new forums and make the old one a “museum”.

My CPU consumption usually varies from 8 to 30. 40-50 on peak days.

Without doing this “hacks” my website can’t survive… because the cpu consumption goes up to 500 until everything crashes.

Note: See TracTickets for help on using tickets.