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.
Comment | File | Size | Author |
---|---|---|---|
#49 | 1354726-49.patch | 874 bytes | damiankloip |
#36 | 1354726-36-tests.patch | 1.51 KB | damiankloip |
#34 | 1354726-34.patch | 873 bytes | damiankloip |
#32 | 1354726-32.patch | 873 bytes | damiankloip |
#29 | 1354726-29.patch | 873 bytes | damiankloip |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe 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.
Comment #2
larowlanBeat me to it, thanks damien
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #4
bvanmeurs CreditAttribution: bvanmeurs commentedInteresting, I didn't think of that. Nevertheless, I disabled the cache and am experiencing no performance problems.
Comment #5
droplet CreditAttribution: droplet commentedYeah, 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
Comment #6
gnugetHi
I added the index on last_comment_timestamp.
I attached the patch.
Comment #7
alexweber CreditAttribution: alexweber commentedComment #8
DevElCuy CreditAttribution: DevElCuy commentedJust added the index in a hook_update_N(), not sure if it is needed.
Comment #9
DevElCuy CreditAttribution: DevElCuy commentedAdding unit test to verify that the index has been created.
Comment #10
droplet CreditAttribution: droplet commentedcan 'forum_active' have a better name. IMO we don't need the unit test.
Comment #11
gnugetWhat would be a better name?
something like: "forum_last_comment_timestamp"?
let me know and i will re-attach a new patch
Comment #12
droplet CreditAttribution: droplet commentedI suggest 'last_comment_timestamp'
Comment #13
gnugetI attach another patch.
I changed the name of the index, i put the name to you suggested.
Also, i remove the test
Comment #14
alexweber CreditAttribution: alexweber commentedgo testbot!
Comment #15
DevElCuy CreditAttribution: DevElCuy commentedComment #16
catchUntil we support head2head updates, the 8.x patch should not have an update function, that needs to go directly into 7.x.
Comment #17
droplet CreditAttribution: droplet commentedalso added 'created'
remove forum blocks caches:#1377520: Remove forum blocks caches
Comment #18
droplet CreditAttribution: droplet commentedComment #19
DevElCuy CreditAttribution: DevElCuy commentedComment #20
catchPlease post before/after EXPLAIN output on the queries these indexes are intended to improve.
Comment #21
DevElCuy CreditAttribution: DevElCuy commentedAdding tag "dlatino" for reference of the Drupal Latino community.
Comment #22
edutrul CreditAttribution: edutrul commentedHere is the query used by the block 'Active forum topics':
Here is the output of this query EXPLAIN without the index:
Here is the output of this query EXPLAIN with the index:
Comment #23
heilop CreditAttribution: heilop commentedHere is the query used by the block 'New forum topics':
Here is the output of this query EXPLAIN without the index:
Here is the output of this query EXPLAIN with the index:
Comment #24
webchickOh awesome! Thanks a lot for those EXPLAINs!
Bumping back to RTBC and back to catch.
Comment #25
catchThanks. 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).
Comment #26
catchComment #27
cha0s CreditAttribution: cha0s commentedRe-rolled.
Comment #28
xjmThanks @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.
Comment #29
damiankloip CreditAttribution: damiankloip commentedHere 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?
Comment #30
damiankloip CreditAttribution: damiankloip commentedComment #32
damiankloip CreditAttribution: damiankloip commentedSorry, this is better.
Comment #33
xjm"comment" is misspelled here. Oops!
See this document for the upgrade path tests:
http://drupal.org/node/1429136
Comment #34
damiankloip CreditAttribution: damiankloip commentedHa, I can spell comments, honest! :)
Here is a patch with that fixed, looking at tests now.
Comment #35
xjmFor the bot.
Comment #36
damiankloip CreditAttribution: damiankloip commentedHere 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...
Comment #38
droplet CreditAttribution: droplet commentedDoes it really need a test case ??
Comment #39
droplet CreditAttribution: droplet commented#34: 1354726-34.patch queued for re-testing.
Comment #40
damiankloip CreditAttribution: damiankloip commented@droplet, not sure tbh, Just doing what I'm told :)
Comment #41
catchWith 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.
Comment #42
damiankloip CreditAttribution: damiankloip commented@catch, no probs. I will have a look at this today.
Comment #43
damiankloip CreditAttribution: damiankloip commentedThere 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.
Comment #44
catchIf 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.
Comment #45
hosef CreditAttribution: hosef commentedI 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.
Comment #46
sunNo, no tests for schema index changes. And this is the wrong issue to write global upgrade schema verification tests.
#34 is ready to go.
Comment #47
damiankloip CreditAttribution: damiankloip commentedThanks sun, that's what I was hoping!
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedPatch 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().
Comment #49
damiankloip CreditAttribution: damiankloip commentedHere is a rerolled patch using forum_update_7012.
Howcome the update hooks have jumped from 7003 to 7011, just out of interest?
Comment #50
damiankloip CreditAttribution: damiankloip commentedComment #51
BerdirLooks good, same patch as #34, just with the different update number.
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/c2fd994
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.
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedAlso just noticed that docblock indentation is messed up, so I fixed it in a quick followup commit.