There's a bug in forum.module that I wanted to fix this cycle, and I realize I am not going to get to it. I've got entirely too many things on my plate (Views 2, I'm looking at you) and I need to divest myself of some of these things I want to do. However, this one is important. template_preprocess_forum_topic_navigation (formerly theme_forum_topic_navigation) has this *really* awful query in it: function template_preprocess_forum_topic_navigation(&$variables) { $output = ''; // get previous and next topic $sql = "SELECT n.nid, n.title, n.sticky, l.comment_count, l.last_comment_timestamp FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1 ORDER BY n.sticky DESC, ". _forum_get_topic_order_sql(variable_get('forum_order', 1)); $result = db_query(db_rewrite_sql($sql), isset($variables['node']->tid) ? $variables['node']->tid : 0); $stop = $variables['prev'] = $variables['next'] = 0; while ($topic = db_fetch_object($result)) { if ($stop == 1) { $variables['next'] = $topic->nid; $variables['next_title'] = check_plain($topic->title); $variables['next_url'] = url("node/$topic->nid"); break; } if ($topic->nid == $variables['node']->nid) { $stop = 1; } else { $variables['prev'] = $topic->nid; $variables['prev_title'] = check_plain($topic->title); $variables['prev_url'] = url("node/$topic->nid"); } } } To summarize: It runs a query to find every post in the current forum, then counts its way through to your current post, then looks 1 ahead to see what the next/previous posts are. If you have thousands of topics in your forum, this is extremely inefficient. This can be much more efficiently done in two queries where you do a limit 1 and do > on one and < on the other, using the sort criteria, to determine what next/previous is. For example, if you're sorting on created, you do a SELECT nid FROM node WHERE [sufficient clauses to restrict to just this forum] WHERE node.created >= $node->created AND node.nid <> $node->nid LIMIT 1 And reverse it to find the previous. Here's the challenge: Write the patch to fix this, cause this needs to happen. I checked the issue queue and I don't see an issue for it.
Earl Miles wrote: template_preprocess_forum
_topic_navigation (formerly theme_forum_topic_navigation) has this *really* awful query in it:
[..] Here's the challenge: Write the patch to fix this, cause this needs to happen. I checked the issue queue and I don't see an issue for it. _________ I think this is the issue (although nothing in there for that particular query). http://drupal.org/node/145353 (forum performance improvements) I have an abortive patch on there (the forum table disappeared as I was working on it) which was a start on: - forum_get_topics - forum_get_forums - forum_topics_unread Also: http://drupal.org/node/148849 (merge {node_comment_statistics} and {node_counter} into {node} which'd affect that query. catch There's a bug in forum.module that I wanted to fix this cycle, and I realize
I am not going to get to it. I've got entirely too many things on my plate (Views 2, I'm looking at you) and I need to divest myself of some of these things I want to do. However, this one is important.
template_preprocess_forum_topic_navigation (formerly theme_forum_topic_navigation) has this *really* awful query in it:
function template_preprocess_forum_topic_navigation(&$variables) { $output = '';
// get previous and next topic $sql = "SELECT n.nid, n.title, n.sticky, l.comment_count, l.last_comment_timestamp FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1 ORDER BY n.sticky DESC, ". _forum_get_topic_order_sql(variable_get('forum_order', 1)); $result = db_query(db_rewrite_sql($sql), isset($variables['node']->tid) ? $variables['node']->tid : 0);
$stop = $variables['prev'] = $variables['next'] = 0;
while ($topic = db_fetch_object($result)) { if ($stop == 1) { $variables['next'] = $topic->nid; $variables['next_title'] = check_plain($topic->title); $variables['next_url'] = url("node/$topic->nid"); break; } if ($topic->nid == $variables['node']->nid) { $stop = 1; } else { $variables['prev'] = $topic->nid; $variables['prev_title'] = check_plain($topic->title); $variables['prev_url'] = url("node/$topic->nid"); } } }
To summarize: It runs a query to find every post in the current forum, then counts its way through to your current post, then looks 1 ahead to see what the next/previous posts are. If you have thousands of topics in your forum, this is extremely inefficient.
This can be much more efficiently done in two queries where you do a limit 1 and do > on one and < on the other, using the sort criteria, to determine what next/previous is.
For example, if you're sorting on created, you do a SELECT nid FROM node WHERE [sufficient clauses to restrict to just this forum] WHERE node.created >= $node->created AND node.nid <> $node->nid LIMIT 1
And reverse it to find the previous.
Here's the challenge: Write the patch to fix this, cause this needs to happen. I checked the issue queue and I don't see an issue for it.
On 24 Jul 2007, at 12:03, catch libcom wrote:
Here's the challenge: Write the patch to fix this, cause this needs to happen. I checked the issue queue and I don't see an issue for it.
Talking of which (and I don't intend to hijack this thread): I'm busy compiling an overview of all the pending performance patches that can make a difference on drupal.org. If you can send me your lists, then I'll prioritize them. If you intend to carry one of these patches home, please let us know as well. It'd love to assign names to each patch, and organize us around this work. Improving the forum module queries, both the query mentioned by Earl and the query responsible for the 'Active forum topics' block, would be of tremendous impact for drupal.org. Therefore, I'd like to see us spend more time working on improving the performance of CVS HEAD (and DRUPAL-5). These patches can still go into Drupal 6, even if that requires breaking some APIs. We need to pick up this work now and get these patches in as soon as possible. After August 1st, it is probably too late to break APIs, even for performance's sake. So please send me your favorite issues (especially if you plan to work on them more), and I'll trim the list to a handful of patches that we can collectively focus on the next couple of week. I'd like to ask all of you to allocate some extra time to getting these performance improvements in. Thanks for your cooperation, -- Dries Buytaert :: http://www.buytaert.net/
Regarding d.o., I've been wondering about the apparent increase in downtime since update_status started to be widespread: is there any way to check whether it is responsible for a significant part of the increased server load ? ----- Original Message ----- From: "Dries Buytaert" <dries.buytaert@gmail.com> To: <development@drupal.org> Sent: Tuesday, July 24, 2007 6:35 PM Subject: Re: [development] Forum module bug challenge [...]
Talking of which (and I don't intend to hijack this thread): I'm busy compiling an overview of all the pending performance patches that can make a difference on drupal.org [...]
On Jul 24, 2007, at 10:53 AM, FGM wrote:
I've been wondering about the apparent increase in downtime since update_status started to be widespread: is there any way to check whether it is responsible for a significant part of the increased server load ?
The overwhelming majority of the d.o downtime has been from a slammed DB server. For the server side of update(_status?).module, the DB is only touched once every 6 hours to regenerate the .xml files for the release history for each project. These queries don't show up in the top N list of most expensive queries on the DB server. To actually serve the history, there's no Drupal bootstrap at all, just an ultra- thin PHP wrapper. So, to answer your question: "Yes, we checked. No, it's not responsible at all". Cheers, -Derek (dww)
Dries Buytaert a ecrit le 24/07/2007 18:35:
Talking of which (and I don't intend to hijack this thread): I'm busy compiling an overview of all the pending performance patches that can make a difference on drupal.org. If you can send me your lists, then I'll prioritize them. If you intend to carry one of these patches home, please let us know as well. It'd love to assign names to each patch, and organize us around this work.
There's the block cache patch in http://drupal.org/node/80951. Works quite right (probably needs a re-roll, though), but is currently stuck on the way to handle node_access rights. The simplest path is "no block caching when n_a modules are used". Sad, but we still gain a lot on non n_a sites. Other possibilities include some sort of user defined settings through the UI. Opinions welcome... Yched
On 7/24/07, Earl Miles <merlin@logrus.com> wrote:
This can be much more efficiently done in two queries where you do a limit 1 and do > on one and < on the other, using the sort criteria, to determine what next/previous is.
For example, if you're sorting on created, you do a SELECT nid FROM node WHERE [sufficient clauses to restrict to just this forum] WHERE node.created >= $node->created AND node.nid <> $node->nid LIMIT 1
And reverse it to find the previous.
I did something similar for a client. They wanted to display the thumbnails of the next/previous image below each image node. What I did is go by the node ID. So this function does what you mentioned above. function pn_node($node, $mode) { switch($mode) { case 'p': $op = '<'; $order = 'DESC'; break; case 'n': $op = '>'; $order = 'ASC'; break; default: return NULL; } $sql = "SELECT n.nid FROM {node} n WHERE n.nid $op %d AND n.type in ('image', 'video') AND n.status = 1 AND n.promote = 1 ORDER BY n.nid $order LIMIT 1"; $n_nid = db_result(db_query($sql, $node->nid)); if ($n_nid) { $n_node = node_load($n_nid); if ($n_node->type == 'image') { return l(image_display($n_node, 'thumbnail'), "node/$n_nid", array(), NULL, NULL, FALSE, TRUE); } } } Then in the node-image.tpl.php file, I have this: <?php if ($page) { print pn_node($node, 'n'); print pn_node($node, 'p'); } ?> It is not a bottleneck as far as queries go, but it does get executed a lot. The issue with forums is that you have to join the term_node too and filter by the forum, which will add to the query time. -- 2bits.com http://2bits.com Drupal development, customization and consulting.
Khalid Baheyeldin wrote:
On 7/24/07, *Earl Miles* <merlin@logrus.com <mailto:merlin@logrus.com>> wrote:
It is not a bottleneck as far as queries go, but it does get executed a lot.
The issue with forums is that you have to join the term_node too and filter by the forum, which will add to the query time.
The other issue is that there are 4 different fields that can be sorted on, so the query has to be fairly smart.
Ooooh - that's REALLY useful to me. How would you take that one step further and limit it to the current category? Khalid Baheyeldin wrote:
On 7/24/07, *Earl Miles* <merlin@logrus.com <mailto:merlin@logrus.com>> wrote:
This can be much more efficiently done in two queries where you do a limit 1 and do > on one and < on the other, using the sort criteria, to determine what next/previous is.
For example, if you're sorting on created, you do a SELECT nid FROM node WHERE [sufficient clauses to restrict to just this forum] WHERE node.created >= $node->created AND node.nid <> $node->nid LIMIT 1
And reverse it to find the previous.
I did something similar for a client. They wanted to display the thumbnails of the next/previous image below each image node.
What I did is go by the node ID.
So this function does what you mentioned above.
function pn_node($node, $mode) { switch($mode) { case 'p': $op = '<'; $order = 'DESC'; break;
case 'n': $op = '>'; $order = 'ASC'; break;
default: return NULL; }
$sql = "SELECT n.nid FROM {node} n WHERE n.nid $op %d AND n.type in ('image', 'video') AND n.status = 1 AND n.promote = 1 ORDER BY n.nid $order LIMIT 1";
$n_nid = db_result(db_query($sql, $node->nid)); if ($n_nid) { $n_node = node_load($n_nid); if ($n_node->type == 'image') { return l(image_display($n_node, 'thumbnail'), "node/$n_nid", array(), NULL, NULL, FALSE, TRUE); } } }
Then in the node-image.tpl.php file, I have this:
<?php if ($page) { print pn_node($node, 'n'); print pn_node($node, 'p'); } ?>
It is not a bottleneck as far as queries go, but it does get executed a lot.
The issue with forums is that you have to join the term_node too and filter by the forum, which will add to the query time. -- 2bits.com <http://2bits.com> http://2bits.com Drupal development, customization and consulting.
-- Sean Robertson Web Developer NGP Software, Inc. seanr@ngpsoftware.com (202) 686-9330 http://www.ngpsoftware.com
participants (8)
-
catch libcom -
Derek Wright -
Dries Buytaert -
Earl Miles -
FGM -
Khalid Baheyeldin -
Sean Robertson -
Yves Chedemois