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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Title: Unnecessary cache on active block » Add 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.

larowlan’s picture

Beat me to it, thanks damien

Damien Tournoud’s picture

Title: Add index on {form_index}.last_comment_timestamp » Add index on {forum_index}.last_comment_timestamp
bvanmeurs’s picture

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

droplet’s picture

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

gnuget’s picture

FileSize
478 bytes

Hi

I added the index on last_comment_timestamp.

I attached the patch.

alexweber’s picture

Status: Active » Needs review
DevElCuy’s picture

FileSize
747 bytes

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

DevElCuy’s picture

FileSize
1.4 KB

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

droplet’s picture

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

gnuget’s picture

What would be a better name?

something like: "forum_last_comment_timestamp"?

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

droplet’s picture

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

I suggest 'last_comment_timestamp'

gnuget’s picture

FileSize
767 bytes

I attach another patch.

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

Also, i remove the test

alexweber’s picture

Status: Needs work » Needs review

go testbot!

DevElCuy’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

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.

droplet’s picture

Status: Needs work » Needs review
FileSize
526 bytes

also added 'created'

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

droplet’s picture

Title: Add index on {forum_index}.last_comment_timestamp » Add more indexs on {forum_index}
DevElCuy’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review

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

DevElCuy’s picture

Issue tags: +dlatino

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

edutrul’s picture

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:

heilop’s picture

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:

webchick’s picture

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.

catch’s picture

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).

catch’s picture

Title: Add more indexs on {forum_index} » Add more indexes on {forum_index}
Category: bug » task
cha0s’s picture

Status: Patch (to be ported) » Needs review
FileSize
511 bytes

Re-rolled.

xjm’s picture

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.

damiankloip’s picture

FileSize
873 bytes

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?

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
873 bytes

Sorry, this is better.

xjm’s picture

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

damiankloip’s picture

FileSize
873 bytes

Ha, I can spell comments, honest! :)

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

xjm’s picture

Status: Needs work » Needs review

For the bot.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

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.

droplet’s picture

Does it really need a test case ??

droplet’s picture

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

damiankloip’s picture

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

catch’s picture

With the test I'd prefer to do schema checks generically per #1119094: Add a test to verify the schema matches after a database update, 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.

damiankloip’s picture

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

damiankloip’s picture

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

Are we waiting for #1119094: Add a test to verify the schema matches after a database update 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.

catch’s picture

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.

hosef’s picture

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.

sun’s picture

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.

damiankloip’s picture

Thanks sun, that's what I was hoping!

David_Rothstein’s picture

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().

damiankloip’s picture

FileSize
874 bytes

Here is a rerolled patch using forum_update_7012.

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

damiankloip’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

David_Rothstein’s picture

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.

David_Rothstein’s picture

+ /**
+ * 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.