Merging Post performance issues

[Xon]

Affected version

2.2.6 Patch 2

When merging posts the updateSourceData function ends up doing a lot of work inside a transaction which likely is not required.

This is notable on a very busy forum when merging posts in the same thread, as this can cause deadlocks and cause replying to faill, for the entire node the thread lives in.

Thread::rebuildCounters is called twice, once in updateTargetData and once in updateSourceData.

Additionally;

In updateTargetData

• The call to Thread::rebuildCounters is redundant
  • When source thread != target thread; the target post doesn't move it's position in the thread.
  • When source thread = target thread, then the thread metadata is updated when the source post(s) are deleted.

In updateSourceData:

• Thread::rebuildCounters is called even if source thread is the same as the target thread, which causes un-needed scans of the xf_post table and possible other extensive rebuild operation depending on add-ons.
  • This information is updated when the source posts are deleted, so again; completely redundant.
• Calls to rebuildThreadPostPositions/rebuildThreadUserPostCounters/Forum::rebuildCounters are redundant as the deleting the source post(s) updates this information.

In cleanupActions

• Querying indexing the deleted posts isn't required, as this is done in source post's Post::Delete method.
• Calls to rebuildThreadPostPositions/rebuildThreadUserPostCounters is redundant;
  • When source thread != target thread; the target post doesn't move it's position in the thread.
  • When source thread = target thread, then the thread metadata is updated when the source post(s) are deleted.

When the source post(s) are deleted, this will hold a number of write locks on various thread and forum rows; which increases the rick of deadlocks as updateTargetData and updateSourceData are now holding write locks on all posts in the source & target thread, and the source forum(s).

 

[Xon]

The same thing applies to XF\Service\Post\Copier::updateTargetData for calls to Thread::rebuildCounters/rebuildThreadPostPositions/rebuildThreadUserPostCounters/Forum::rebuildCounters are redundant, as saving the new post triggers nearly all of this anyway.

I think the only issue is rebuildThreadPostPositions is not redundant as the new post is inserted with position=0.

 

[Xon]

XF\Service\Post\Merger / XF\Service\Post\Mover's updateSourceData can also trigger Forum::rebuildCounters multiple times more than needed (as it is called once per thread rather than once per forum) inside the transaction

 

[PaulB]

Part of the issue here appears to be that none of the applicable indices on xf_post include message_state. Most page views need to filter on message_state, and Thread#rebuildReplyCount filters on message_state as well; those queries are going to be much slower than necessary. Both require counting based on at least one criterion that isn't included in the index, and counting that way is quite slow.

 

[Xon]

There is also an unexpected filesort for queries hitting thread_id/position/post_date (by thread_id, sort position + post_date)

The old thread_id_position index pre-dates adding post_date to ensure user-visible consistent sorting when the position is identical.

Probably need to remove it and add an thread_id_position_post_date index

 

[PaulB]

Might be better to keep the indices the way they are and use post_id instead of post_date as the last column in the ORDER BY clause. Reasoning:

1. post_id is already at the end of the thread_id_position index, albeit implicitly.
2. post_date isn't going to preserve order anyway in popular threads that have replies within a second of each other.
3. post_id isn't perfect, but it's good enough here, since the post_id ordering is only applicable to hidden posts. Any hidden posts that are between the same two visible posts will retain that positioning; they just might be out-of-order relative to each other.