Skip to:
Content

Opened 21 months ago

Last modified 6 days ago

#1925 new enhancement

Optimize `_bbp_last_active_time` to use `post_modified` instead

Reported by: vibol Owned by:
Milestone: 2.6 Priority: normal
Severity: normal Version: 2.1.2
Component: Performance Improvements 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 20 months ago.
1925.diff (8.7 KB) - added by MZAWeb 16 months ago.
1925.bbPress1.php.diff (624 bytes) - added by netweb 8 months ago.
1925.003.patch (14.6 KB) - added by thebrandonallen 7 months ago.
Read and write post_modified. No database upgrading.
1925-thebrandonallen.png (56.6 KB) - added by netweb 4 months ago.
1925-mzaweb.png (51.4 KB) - added by netweb 4 months ago.
1925-mzaweb-wp_posts.sql (5.4 KB) - added by netweb 4 months ago.
1925-thebrandonallen-wp_posts.sql (7.3 KB) - added by netweb 4 months ago.
1925.04.diff (17.9 KB) - added by netweb 4 months ago.
1925.05.diff (17.9 KB) - added by netweb 4 months ago.
0001-bbPress-performance-enhancements-http-bbpress.trac.w.patch (11.2 KB) - added by vibol 4 months ago.
1925.patch (10.4 KB) - added by johnjamesjacoby 4 months ago.
Includes _update_ functions, removes phpdoc changes from previous patches
1925.2.patch (10.2 KB) - added by netweb 4 months ago.
Fixed Copy Pasta Error ;)
1925.converters.patch (17.3 KB) - added by netweb 4 months ago.
Updated converters:
1925.post_modified.01.diff (17.6 KB) - added by thebrandonallen 4 months 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 4 months ago.
1925.4.diff (25.6 KB) - added by thebrandonallen 3 months ago.
1925.alt.01.diff (8.9 KB) - added by netweb 3 months ago.

Download all attachments as: .zip

Change History (93)

comment:1 Mamaduka21 months ago

  • Cc georgemamadashvili@… added

comment:2 follow-up: johnjamesjacoby21 months 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.

comment:3 vibol20 months 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.

vibol20 months ago

comment:4 MZAWeb17 months 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 17 months ago by MZAWeb (previous) (diff)

comment:5 MZAWeb17 months ago

  • Cc wordpress@… added

comment:6 johnjamesjacoby17 months 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).

comment:7 johnjamesjacoby17 months ago

  • Milestone changed from 2.3 to 2.4

comment:8 MZAWeb16 months 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.

MZAWeb16 months ago

comment:9 follow-ups: MZAWeb16 months 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)

comment:10 vibol15 months 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.

comment:11 vibol14 months 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.

comment:12 jaredatch13 months ago

  • Cc jared@… added

comment:13 in reply to: ↑ 9 netweb13 months 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.

comment:14 follow-up: alex-ye13 months 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 .

comment:15 in reply to: ↑ 14 ; follow-up: MZAWeb13 months 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.

comment:16 in reply to: ↑ 15 alex-ye13 months 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 ?

comment:17 johnjamesjacoby11 months 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.

comment:18 ethitter8 months ago

  • Cc erick@… added

netweb8 months ago

comment:19 in reply to: ↑ 9 netweb8 months 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.

comment:20 follow-up: thebrandonallen7 months 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.

thebrandonallen7 months ago

Read and write post_modified. No database upgrading.

comment:21 johnjamesjacoby5 months 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?

comment:22 johnjamesjacoby4 months ago

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

comment:23 johnjamesjacoby4 months 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

comment:24 in reply to: ↑ 2 netweb4 months 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.

comment:25 netweb4 months 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.

netweb4 months ago

netweb4 months ago

comment:26 netweb4 months 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 4 months ago by netweb (previous) (diff)

comment:27 netweb4 months 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

comment:28 follow-up: netweb4 months 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?

comment:29 in reply to: ↑ 20 netweb4 months 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.

netweb4 months ago

comment:30 netweb4 months ago

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

comment:31 netweb4 months 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.

comment:32 netweb4 months 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.

comment:33 in reply to: ↑ 28 ; follow-up: thebrandonallen4 months 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!

comment:34 in reply to: ↑ 33 netweb4 months 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 4 months ago by netweb (previous) (diff)

netweb4 months ago

comment:35 netweb4 months 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.

netweb4 months ago

comment:36 netweb4 months 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

comment:37 netweb4 months 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

comment:38 follow-up: vibol4 months 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.

comment:39 in reply to: ↑ 38 netweb4 months 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.

comment:40 netweb4 months ago

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

comment:41 follow-up: thebrandonallen4 months 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 4 months ago by thebrandonallen (previous) (diff)

comment:42 johnjamesjacoby4 months 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.

comment:43 johnjamesjacoby4 months 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.

comment:44 johnjamesjacoby4 months 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.

comment:45 follow-up: thebrandonallen4 months 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.

comment:46 johnjamesjacoby4 months 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.

johnjamesjacoby4 months ago

Includes _update_ functions, removes phpdoc changes from previous patches

comment:47 thebrandonallen4 months 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

comment:48 thebrandonallen4 months 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.

comment:49 in reply to: ↑ 41 netweb4 months 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 4 months ago by netweb (previous) (diff)

netweb4 months ago

Fixed Copy Pasta Error ;)

netweb4 months ago

Updated converters:

comment:50 netweb4 months 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)

comment:51 follow-up: thebrandonallen4 months 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.

thebrandonallen4 months ago

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

johnjamesjacoby4 months ago

comment:52 in reply to: ↑ 51 johnjamesjacoby4 months 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.

comment:53 follow-up: johnjamesjacoby4 months ago

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

comment:54 in reply to: ↑ 53 thebrandonallen4 months 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.

comment:55 netweb4 months 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

comment:56 follow-up: netweb4 months 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');

comment:57 in reply to: ↑ 56 thebrandonallen4 months 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.

comment:58 mordauk3 months ago

  • Cc pippin@… added

comment:59 follow-up: netweb3 months 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

comment:60 ircbot3 months ago

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

comment:61 in reply to: ↑ 45 netweb3 months 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 :)

comment:62 in reply to: ↑ 59 thebrandonallen3 months 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.

comment:63 follow-up: johnjamesjacoby3 months 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.

comment:64 thebrandonallen3 months 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.

thebrandonallen3 months ago

comment:65 in reply to: ↑ 63 netweb3 months 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 :)

comment:66 thebrandonallen3 months 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.

netweb3 months ago

comment:67 netweb3 months 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' ) )

comment:68 sc0ttkclark2 months 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).

comment:69 johnjamesjacoby2 months 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.

comment:70 sc0ttkclark2 months ago

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

comment:71 thebrandonallen2 months 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.

comment:72 sc0ttkclark2 months 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' );

comment:73 sc0ttkclark2 months ago

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

comment:74 ircbot2 months ago

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

comment:75 netweb6 days 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.

Note: See TracTickets for help on using tickets.