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.

Comments

michelle’s picture

Category: bug » feature
Status: Active » Postponed (maintainer needs more info)

Yes, 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

ashtronaut’s picture

I 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

jgoldfeder’s picture

It takes over a minute to run with 300,000+ nodes.

maulwuff’s picture

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

SELECT
        n.title   AS nodetitle,
        res.title AS restitle ,
        res.created           ,
        res.type AS restype   ,
        n.type   AS nodetype  ,
        n.nid    AS nid       ,
        t.tid                 ,
        res.cid AS cid
FROM
        node AS n
        INNER JOIN
                (SELECT
                        title  ,
                        created,
                        nid    ,
                        uid    ,
                        type   ,
                        'cid' AS cid
                FROM
                        node
                WHERE
                        type    ='forum'
                    AND created > (unix_timestamp() - 24*60*60 * 365)
                
                UNION
                
                SELECT
                        subject  ,
                        TIMESTAMP,
                        nid      ,
                        uid      ,
                        'comment',
                        cid
                FROM
                        comments
                WHERE
                        TIMESTAMP > (unix_timestamp() - 24*60*60 * 365)
                ORDER BY
                        created DESC
                ) AS res
        ON
                n.nid=res.nid
        INNER JOIN term_node AS t
        ON
                n.nid = t.nid
WHERE
        n.type = 'forum'
GROUP BY
        tid;
bardkerbie’s picture

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

jono@ipman.co.za’s picture

ummm, still getting 30-39 secs for forum with 7000 topics & 450 000 comments.

michelle’s picture

Title: Extremely slow query in advanced_forum_get_all_last_topics() » Create table for advanced_forum_get_all_last_topics()
Version: 5.x-1.0-alpha7 » 5.x-1.x-dev
Assigned: Unassigned » michelle
Category: feature » task
Status: Postponed (maintainer needs more info) » Active

Ok, 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

jaydub’s picture

I'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):

SELECT
  f.nid, f.tid, n.title, u.name, n.created
FROM 
  forum f 
INNER JOIN 
  node n
ON 
  f.nid = n.nid
INNER JOIN 
  users u
ON
  n.uid = u.uid 
GROUP BY 
  f.tid 
ORDER BY 
  n.created DESC;

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;

SELECT
  f.nid, f.tid, c.cid, c.subject,
  u.name, c.timestamp
FROM  
  forum f 
INNER JOIN 
  comments c 
ON 
  f.nid = c.nid 
INNER JOIN 
  users u 
ON 
  c.uid = u.uid 
GROUP BY 
  f.tid  
ORDER BY 
  c.timestamp DESC;

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?

michelle’s picture

"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

michelle’s picture

I guess it must be more complicated than that because those queries aren't returning the right posts. :(

Thanks for trying,

Michelle

jaydub’s picture

Status: Active » Needs review
StatusFileSize
new4.23 KB

How 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?

michelle’s picture

Thanks 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

liakoni’s picture

I 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 ?

jaydub’s picture

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

ashtronaut’s picture

RE: #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

jaydub’s picture

StatusFileSize
new4.27 KB

Ok re-rolled against current version 1.56 of advanced_forum.module

ashtronaut’s picture

Patch 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

michelle’s picture

Status: Needs review » Needs work

Ok, 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

michelle’s picture

Status: Needs work » Active

Getting 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

michelle’s picture

Ok, 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:

  1. Account for deleted/unpublished/access control on comments and nodes.
  2. Make comment links work on multi page threads
  3. Write something in the install file to backfill the table with existing posts.
  4. Document variable changes in .tpl
  5. Add graphic link to comment for people who don't use comment titles
  6. Mucho testing. I got as far as it looking like it works and now the baby's up so I committed what I have as a start.
  7. Port to 6.x

Michelle

michelle’s picture

Ok, 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

michelle’s picture

Committed #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

michelle’s picture

#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

maulwuff’s picture

what do you mean with "comment titles" ? do you need an invisible spacer, or an alternative for i.e. comment "#9" ?

michelle’s picture

@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

maulwuff’s picture

ok, I see what you mean. I'll give it a try.

hmm, looks strange. it's black and white only.

maulwuff’s picture

StatusFileSize
new612 bytes

umm, drupal ate the attachment. :)

michelle’s picture

Status: Active » Fixed

Thanks 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

brookvale’s picture

I 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?

michelle’s picture

The alpha is already out.

Michelle

brookvale’s picture

Michelle,
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?

brookvale’s picture

Also:
Template.php with the $vars code (per instructions) was put into Bluemarine theme folder but still the posts are in default Drupal style.

sgdev’s picture

See the thread here: http://drupal.org/node/267827

Anonymous’s picture

Status: Fixed » Closed (fixed)

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