The forum active block shows the latest active forum threads. It uses caching in the render element, and caches a query using this mechanism. This means that the latest forum threads won't show directly. I don't see why this caching is necessary, because the forum_index table is searched on the 'last_comment_timestamp'. This column isn't indexed, but if you index it the query is so damn fast that caching it is nonesense.

Solution: put an index where it belongs: on the last_comment_timestamp of the forum_index table. Then remove the cache and just query and parse the results in every block view.

Files: 
CommentFileSizeAuthor
#49 1354726-49.patch874 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 39,108 pass(es).
[ View ]
#36 1354726-36-tests.patch1.51 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1354726-36-tests.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 1354726-34.patch873 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 39,068 pass(es).
[ View ]
#32 1354726-32.patch873 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 38,697 pass(es).
[ View ]
#29 1354726-29.patch873 bytesdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/forum/forum.install.
[ View ]
#27 1354726-forum-indices-7.patch511 bytescha0s
PASSED: [[SimpleTest]]: [MySQL] 38,686 pass(es).
[ View ]
#23 created_withoutindex.png5.86 KBheilop
#23 created_withindex.png5.79 KBheilop
#22 explain_noindex.png6.02 KBedutrul
#22 explain_withindex.png6.48 KBedutrul
#17 forum_db_index.patch526 bytesdroplet
PASSED: [[SimpleTest]]: [MySQL] 34,244 pass(es).
[ View ]
#13 forum-1354726-13.patch767 bytesgnuget
PASSED: [[SimpleTest]]: [MySQL] 34,206 pass(es).
[ View ]
#9 forum-1354726-9.patch1.4 KBdevelCuy
PASSED: [[SimpleTest]]: [MySQL] 34,181 pass(es).
[ View ]
#8 forum-1354726-8.patch747 bytesdevelCuy
PASSED: [[SimpleTest]]: [MySQL] 34,158 pass(es).
[ View ]
#6 forum-1354726-6.patch478 bytesgnuget
PASSED: [[SimpleTest]]: [MySQL] 34,166 pass(es).
[ View ]

Comments

Title:Unnecessary cache on active blockAdd index on {form_index}.last_comment_timestamp
Version:7.9» 8.x-dev
Category:feature» bug

We should add an index here, but it's not the point of the cache. The point of the cache is that when you add access queries to the mix, this query is going to be slow regardless of what we do.

Beat me to it, thanks damien

Title:Add index on {form_index}.last_comment_timestampAdd index on {forum_index}.last_comment_timestamp

Interesting, I didn't think of that. Nevertheless, I disabled the cache and am experiencing no performance problems.

Yeah, I did some test when I work on following issues. with 1M nodes, there is slightly differences only, no big performance problem. however I haven't post the result there. It still worth to add an index :)

http://drupal.org/node/914382#comment-3973382
#1008410: Forum list header sorting broken

StatusFileSize
new478 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,166 pass(es).
[ View ]

Hi

I added the index on last_comment_timestamp.

I attached the patch.

Status:Active» Needs review

StatusFileSize
new747 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,158 pass(es).
[ View ]

Just added the index in a hook_update_N(), not sure if it is needed.

StatusFileSize
new1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 34,181 pass(es).
[ View ]

Adding unit test to verify that the index has been created.

can 'forum_active' have a better name. IMO we don't need the unit test.

What would be a better name?

something like: "forum_last_comment_timestamp"?

let me know and i will re-attach a new patch

Status:Needs review» Needs work
Issue tags:+needs backport to D7

I suggest 'last_comment_timestamp'

StatusFileSize
new767 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,206 pass(es).
[ View ]

I attach another patch.

I changed the name of the index, i put the name to you suggested.

Also, i remove the test

Status:Needs work» Needs review

go testbot!

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Until we support head2head updates, the 8.x patch should not have an update function, that needs to go directly into 7.x.

Status:Needs work» Needs review
StatusFileSize
new526 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,244 pass(es).
[ View ]

also added 'created'

remove forum blocks caches:#1377520: Remove forum blocks caches

Title:Add index on {forum_index}.last_comment_timestampAdd more indexs on {forum_index}

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs review

Please post before/after EXPLAIN output on the queries these indexes are intended to improve.

Issue tags:+dlatino

Adding tag "dlatino" for reference of the Drupal Latino community.

StatusFileSize
new6.48 KB
new6.02 KB

Here is the query used by the block 'Active forum topics':

SELECT f.* FROM forum_index f ORDER BY f.last_comment_timestamp DESC LIMIT 5 OFFSET 0;

Here is the output of this query EXPLAIN without the index:

Here is the output of this query EXPLAIN with the index:

StatusFileSize
new5.79 KB
new5.86 KB

Here is the query used by the block 'New forum topics':

SELECT f.* FROM forum_index f ORDER BY f.created DESC LIMIT 5 OFFSET 0;

Here is the output of this query EXPLAIN without the index:

Here is the output of this query EXPLAIN with the index:

Assigned:Unassigned» catch
Status:Needs review» Reviewed & tested by the community

Oh awesome! Thanks a lot for those EXPLAINs!

Bumping back to RTBC and back to catch.

Version:8.x-dev» 7.x-dev
Priority:Normal» Critical
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks. Committed/pushed to 8.x, moving this to 7.x for backport and bumping to critical since we need to either commit it or decide not to for 7.x before 8.x supports an upgrade path (since there's no updated in the 8.x version).

Title:Add more indexs on {forum_index}Add more indexes on {forum_index}
Category:bug» task

Status:Patch (to be ported)» Needs review
StatusFileSize
new511 bytes
PASSED: [[SimpleTest]]: [MySQL] 38,686 pass(es).
[ View ]

Re-rolled.

Status:Needs review» Needs work
Issue tags:+Needs tests

Thanks @cha0s for the backport!

If we are backporting this we additionally need an update hook and an accompanying upgrade path test. Marking Needs Work for those additions.

StatusFileSize
new873 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/forum/forum.install.
[ View ]

Here is a patch that includes the update hook. @xjm, let me know what I need to do for the tests and happy to look at that. Do you mean and upgrade or update path test case?

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1354726-29.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new873 bytes
PASSED: [[SimpleTest]]: [MySQL] 38,697 pass(es).
[ View ]

Sorry, this is better.

Status:Needs review» Needs work

+++ b/modules/forum/forum.installundefined
@@ -440,5 +442,13 @@ function forum_update_7003() {
+ * Add 'created' and 'last_commnet_timestamp' indexes.

"comment" is misspelled here. Oops!

See this document for the upgrade path tests:
http://drupal.org/node/1429136

StatusFileSize
new873 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,068 pass(es).
[ View ]

Ha, I can spell comments, honest! :)

Here is a patch with that fixed, looking at tests now.

Status:Needs work» Needs review

For the bot.

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1354726-36-tests.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is an initial patch for upgrade path tests. Currently, I think there is a problem with the test of the forum module not being enabled. Something to do with the dd test dump being used or something? Some pointer would be much appreciated. Thought it would be better to post some work, rather that nothing...

Assigned:catch» damiankloip
Status:Needs review» Needs work

The last submitted patch, 1354726-36-tests.patch, failed testing.

Does it really need a test case ??

#34: 1354726-34.patch queued for re-testing.

@droplet, not sure tbh, Just doing what I'm told :)

With the test I'd prefer to do schema checks generically per #1119094: Add a test to verify schema matches after 7-8 upgrade, also it should probably go in modules/simpletest/tests/upgrade/forum.test since that's where other upgrade path tests live at the moment. Looks like the test itself might be missing a $this->performUpgrade()?

So the thing to check here would be to whether we currently have 7.x-7.x upgrade tests that include the forum module and some forum content - if we do then we don't need a test, if not then it'd be good to add that.

@catch, no probs. I will have a look at this today.

There is a performUpgrade call in the patch (first assertion).

Are we waiting for #1119094: Add a test to verify schema matches after 7-8 upgrade then? I'm confused. Sorry! I am happy to move to the system/tests/ dir where the other are etc... But might want to wait first.

If there's already a 7.x-7.x upgrade test with some forum data in it, I don't think we need explicit test coverage just to check the schema (because we should eventually be able to do that generically for the entire schema).

If there's not any test coverage of 7.x-7.x forum upgrades, then it would make sense to add some forum data to the existing 7.x database dumps, since this issue is only adding an index I'd also be happy to add that in a follow-up issue although webchick might disagree.

I just looked through the D7 test directory, and there is some data in 'drupal-6.forum.database.php' which is used by 'upgrade.forum.test'. There does not appear to be any forum data in 'drupal-7.filled.standard_all.database.php.gz' however. So, it looks like the 6 to 7 upgrade is mostly tested at the moment. However, since the change might affect sites upgrading from 6 to 7, it would probably be a good idea to improve upon these tests so that they cover this change.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

No, no tests for schema index changes. And this is the wrong issue to write global upgrade schema verification tests.

#34 is ready to go.

Thanks sun, that's what I was hoping!

Status:Reviewed & tested by the community» Needs work

Patch looks good, but needs a reroll. We now have forum_update_7011() in the codebase (was introduced in the Drupal 7.13 security update), so the next one should be forum_update_7012().

StatusFileSize
new874 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,108 pass(es).
[ View ]

Here is a rerolled patch using forum_update_7012.

Howcome the update hooks have jumped from 7003 to 7011, just out of interest?

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Looks good, same patch as #34, just with the different update number.

Status:Reviewed & tested by the community» Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/c2fd994

Howcome the update hooks have jumped from 7003 to 7011, just out of interest?

No idea. I assume it was done accidentally during the security update... Nothing we can do about it now, though, since it's already been released that way.

+ /**
+ * Add 'created' and 'last_comment_timestamp' indexes.
+ */

Also just noticed that docblock indentation is messed up, so I fixed it in a quick followup commit.

Automatically closed -- issue fixed for 2 weeks with no activity.