Closed (fixed)
Project:
Advanced Forum
Version:
5.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
2 Apr 2008 at 21:49 UTC
Updated:
21 Jun 2008 at 12:42 UTC
Jump to comment: Most recent file
Hi
I think query in advanced_forum_get_all_last_topics needs to be optimized. Today I've installed advanced forum on a server with 8000 views/day and a lots of comments/nodes. Server become unavailable in a few min after enabling advanced forum module.
I've investigated slow query log and found that query from advanced_forum_get_all_last_topics takes 30 sec (!!!!).
It just trying to select whole comments table (!).
Please test it with explain and you'll see what i mean.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | jumpnew.png | 612 bytes | maulwuff |
| #16 | advanced_forum.module.241982.patch | 4.27 KB | jaydub |
| #11 | advanced_forum.module.241982.patch | 4.23 KB | jaydub |
Comments
Comment #1
michelleYes, it's a nasty query, which is probably why the feature isn't in core. I tried caching the thing and there's an issue around here somewhere detailing all my misery with that. If you're able to optimize it, that's great. Just attach the patch to the issue and I'll be happy to apply it.
Michelle
Comment #2
ashtronaut commentedI am having the same problem with this query on a large site. It takes about 20 sec to run. I'm pretty sure the UNION has something to do with it. I'll play with it some more and see what I can come up with.
ash
Comment #3
jgoldfeder commentedIt takes over a minute to run with 300,000+ nodes.
Comment #4
maulwuff commentedI had a look at the query and found a time-saving-but-not-so-correct-workaround.
The query gets limited to the last 365 days, which saves me about 20sec.
If there is a forum where the oldest answer is older than 365 days, there will be no entry in the column for the last topic.
With some extra code, we could write "too old" or something at these places.
What's different:
I added a where type = forum for the node table and
a time restriction for both, node and comments. If you only add it to comments, there will be wrong topics for the last answer.
Comment #5
bardkerbie commentedI saw a speedup by changing the query from SELECT to SELECT DISTINCT -- that stops the query once it's found what it's looking for instead of iterating through all available data.
Comment #6
jono@ipman.co.za commentedummm, still getting 30-39 secs for forum with 7000 topics & 450 000 comments.
Comment #7
michelleOk, Bdragon came up with a plan. Now I just need to find some time to carry it out.
[09:28] Frankly, what *I* would do is maintain a side table
[09:28] that stores the forum tids and the info on the most recent item in that
[09:28] You can just keep it updated by hooking something
[09:29] and then it'd just be a simple SELECT...
[09:30] hook_nodeapi and hook_comment, and if it was forum related, update the entry in the table...
[09:31] tid, title, created, nid, uid, type ('comment' for a comment), cid ('cid' for a node)
[09:31] like the union does...
It will likely be a day or two before I can get to this. If any of you out there with huge forums have objections to this plan, speak now...
Michelle
Comment #8
jaydub commentedI'm assuming that the goal of this function is to be able to
display the 'last post' for a given forum (tid) which will either
be the forum node or the latest comment.
Is there any reason not to just do something like this (substituting
other desired fields as necessary):
This is for the forum nodes obviously. You can run the query and build
and array in PHP with the details ($tid as the key).
Now get the same results for comments;
Store the results from that query in an array with the $tid as the
key.
Now you iterate on the node results array by the $tid and compare
the node created time against the comment timestamp time and
build your new array with the latest time for each $tid.
Am I oversimplifying this?
Comment #9
michelle"Am I oversimplifying this?"
Hmm... Dunno. :) I didn't write the query to begin with. I'm relatively new at writing Drupal modules so I just assumed that query was so complicated because it has to be. I'll give your suggestion a try and see if it works.
Thanks,
Michelle
Comment #10
michelleI guess it must be more complicated than that because those queries aren't returning the right posts. :(
Thanks for trying,
Michelle
Comment #11
jaydub commentedHow about you try out this patch? I think my approach
is sound and it delivers the expected results for me.
Of course 1 person's experience is not a fully tested
result.
Anyone want to try this out?
Comment #12
michelleThanks for the patch. I'll give it a shot. When I ran your queries manually, they weren't getting the right posts. I'll try this new patch and see what I get.
Michelle
Comment #13
liakoni commentedI have the same problem with this function .The default forum module doen't have this problem . What is the diffenence between them, for finding the last post ?
Comment #14
jaydub commentedThe default forum module doesn't have this feature at all
so you will not have run into the 'problem'.
This feature was added to try and equal the default functionality
of phpbb and the other popular forum-only packages.
Comment #15
ashtronaut commentedRE: #11
I can't get this patch to apply cleanly. It fails at chunk 2. I have tried to apply this to DEV from CVS, as well as the latest alpha release.
ash
Comment #16
jaydub commentedOk re-rolled against current version 1.56 of advanced_forum.module
Comment #17
ashtronaut commentedPatch applied cleanly this time. . . . .
From my initial testing, it looks like the title of the last comment within the thread is being loaded instead of the title of the actual thread. The original functionality showed the title of the actual thread. I actually think the new approach makes more sense, but we will see what everybody else says. As far as speed goes, it has vastly improved. Good job jaydub. . . . I will continue my testing and drop a line here if i come across any issues.
ash
Comment #18
michelleOk, finally had a chance to try the patch and it still applies but ashtronaut is correct that it's pulling the comment title instead of the node title. Ideally, I'd like it to do both, actually. My plan was to show it like this:
COMMENT TITLE
by AUTHOR
posted in NODE TITLE
DAYS AGO
This would be themeable, of course, but all the pieces need to be there. So I need comment title & link, node title & link, comment author & link, and timestamp.
If you have time to work on this more, that would be great. Otherwise, this is pretty high up in priority for me when I get some kid free time.
Thanks,
Michelle
Comment #19
michelleGetting back to this, it's actually worse. It's not only showing the comment titles but the comments aren't even the right ones. Thanks for trying, but I'm going to go back to the original idea of using a table for this.
Michelle
Comment #20
michelleOk, got a good start. We've got a new table to hold the last node/comment in each forum and it pulls it out and prints it up nicely. Still to do:
Michelle
Comment #21
michelleOk, started with #7 on that list. I ported what I have so far to 6.x. Now I need to work on the rest in both branches. I'm impressed. No one has posted an issue so far complaining that all the last posts say "N/A". :) Are people finally listening to me about not using the devs? LOL
BTW, if anyone is testing this out, please let me know if it doesn't work in some way that's not already on my list above. I've got more work to do on it before I give it a good shakedown.
Michelle
Comment #22
michelleCommitted #1 for the 5.x branch. It's only had minimal testing so I need to go over it more before committing it to 6.x. This whole section of code in both branches hasn't been tested very thoroughly at this point. If anyone finds any problems, please post them here in this issue. In addition to the known issues above, there seems to be a problem with the install file. I've only tested updating, not fresh installs so far.
Michelle
Comment #23
michelle#4 is done and committed.
#3 is done but not committed as I want to give the .install file a thorough test first.
Still left to do are #2 and #5 and, of course, lots of testing.
If anyone has a graphic that can be used for jumping right to the comment for sites that don't use comment titles, I'd appreciate it. Needs to be pretty small, around the size of an average character of text.
Michelle
Comment #24
maulwuff commentedwhat do you mean with "comment titles" ? do you need an invisible spacer, or an alternative for i.e. comment "#9" ?
Comment #25
michelle@maulwuff - Just a little icon like an arrow or something. If you look at stand alone forums, they have a little icon that lets you jump to the latest comment. Right now, it's set up to show the linked title of the latest comment, which I think is a nice feature, but might not be desirable for people who don't use titles on their forum comments. So I wanted to offer the alternative of a linked icon they can use instead.
Michelle
Comment #26
maulwuff commentedok, I see what you mean. I'll give it a try.
hmm, looks strange. it's black and white only.
Comment #27
maulwuff commentedumm, drupal ate the attachment. :)
Comment #28
michelleThanks for the try, maulwuff, but I don't think that fits the default theme very well. I searched around a bit and found a GPL icon I could use.
Ok, everything committed to both branches. It still needs more testing but seems to be working fine on my dev site.
Michelle
Comment #29
brookvale commentedI came here from http://drupal.org/node/267495 seeking a fix for the SQL error - and I 'just' a user, not programmer.
Should I turn off advforum until a new(er) alpha version is available?
Comment #30
michelleThe alpha is already out.
Michelle
Comment #31
brookvale commentedMichelle,
After deactivating and removing Alpha8 (complete folder deletion) I installed Alpha9
Activated the module then ran update.php
Got these messages:
* user warning: You have an error in your SQL syntax. Check the manual that corresponds to your MySQL server version for the right syntax to use near ' 0)' at line 1 query: INSERT INTO advforum_last_post (tid, nid, cid) VALUES(14, , 0) in /home/lroca/public_html/includes/database.mysql.inc on line 172.
* user warning: You have an error in your SQL syntax. Check the manual that corresponds to your MySQL server version for the right syntax to use near ' 0)' at line 1 query: INSERT INTO advforum_last_post (tid, nid, cid) VALUES(11, , 0) in /home/lroca/public_html/includes/database.mysql.inc on line 172.
Updates were attempted. If you see no failures below, you may proceed happily to the administration pages. Otherwise, you may need to update your database manually. All errors have been logged.
* Main page
* Administration pages
The following queries were executed
advanced_forum module
Update #5001
* CREATE TABLE {advforum_last_post} ( tid int unsigned NOT NULL default '0', nid int unsigned NOT NULL default '0', cid int unsigned NOT NULL default '0', PRIMARY KEY (tid) ) /*!40100 DEFAULT CHARACTER SET utf8 */;
Update #5002
* DELETE FROM {advforum_last_post} WHERE tid = 13
* INSERT INTO {advforum_last_post} (tid, nid, cid) VALUES(13, 78, 3)
* DELETE FROM {advforum_last_post} WHERE tid = 14
* Failed: INSERT INTO {advforum_last_post} (tid, nid, cid) VALUES(14, , 0)
* DELETE FROM {advforum_last_post} WHERE tid = 10
* INSERT INTO {advforum_last_post} (tid, nid, cid) VALUES(10, 79, 0)
* DELETE FROM {advforum_last_post} WHERE tid = 11
* Failed: INSERT INTO {advforum_last_post} (tid, nid, cid) VALUES(11, , 0)
Could this be due to my installation or a module problem?
Comment #32
brookvale commentedAlso:
Template.php with the $vars code (per instructions) was put into Bluemarine theme folder but still the posts are in default Drupal style.
Comment #33
sgdev commentedSee the thread here: http://drupal.org/node/267827
Comment #34
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.