I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.
Yup, the query will run, but the intention there is for the query optimizer to recognize the
LIMIT 0 and internally reduce that query down to something that quickly returns an empty set (look for "LIMIT Query Optimization" in the MySQL documentation).
In general, when you spot an odd-looking construct in one of my SMF patches, it's likely because I estimated that the alternative way(s) of doing that thing would have led to a less compact diff. Obviously, I'm not going to favor small diffs over every other concern, but it is something that heavily factors into my decision-making (because, in turn, I believe it's something that factors into theymos' decision-making).
Yes, when such a query is executed, there shouldn't be any rows that are parsed, but some database resources will still be consumed.
I certainly agree that the particular change does prioritize code simplicity, which is fair.
I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.
You're right that a maximally-performant implementation would definitely omit the query entirely. Even a query like "
SELECT 1;" is slower than just about anything done
within PHP, since a network request introduces a (relative) ton of latency.
However, I would consider it a slightly
worse patch if it fixed this because it would increase the code complexity a fair bit, and the only benefit would be saving a few milliseconds on a page that will probably only be accessed a handful of times each day. In fact, if I'd been writing this, I probably would've left the query as-is, since that would've only used a couple of additional miliseconds.
I wouldn't necessarily be worried about the user experience of a single request (which, as you say, will be negligible), but of the additional resources consumed. This is coming from a perspective of not knowing where the resource bottlenecks are if any exist.
It seems that PowerGlove is aware that you prioritize simple changes, which is a fair priority, as a more complex change can be more difficult to test, and has the potential to break things and/or introduce a vulnerability.
I am not an expert on the SMF code base, but if there is a bottleneck in the database, the query could be eliminated with the following change to the getTopic topic in Post.php.
function getTopic()
{
global $topic, $db_prefix, $modSettings, $context;
// If we're only peeking, we don't need to fetch previous posts
if ($context['only_peeking'])
{
$context['previous_posts'] = array();
return;
}
// Calculate the amount of new replies.
$newReplies = empty($_REQUEST['num_replies']) || $context['num_replies'] <= $_REQUEST['num_replies'] ? 0 : $context['num_replies'] - $_REQUEST['num_replies'];
if (isset($_REQUEST['xml']))
$limit = "
LIMIT " . (empty($newReplies) ? '0' : $newReplies);
else
$limit = empty($modSettings['topicSummaryPosts']) ? '' : '
LIMIT ' . (int) $modSettings['topicSummaryPosts'];
// If you're modifying, get only those posts before the current one. (otherwise get all.)
$request = db_query("
SELECT IFNULL(mem.realName, m.posterName) AS posterName, m.posterTime, m.body, m.smileysEnabled, m.ID_MSG
FROM {$db_prefix}messages AS m
LEFT JOIN {$db_prefix}members AS mem ON (mem.ID_MEMBER = m.ID_MEMBER)
WHERE m.ID_TOPIC = $topic" . (isset($_REQUEST['msg']) ? "
AND m.ID_MSG < " . (int) $_REQUEST['msg'] : '') . "
ORDER BY m.ID_MSG DESC$limit", __FILE__, __LINE__);
$context['previous_posts'] = array();
while ($row = mysql_fetch_assoc($request))
{
// Censor, BBC, ...
censorText($row['body']);
$row['body'] = parse_bbc($row['body'], $row['smileysEnabled'], $row['ID_MSG']);
$context['previous_posts'][] = $row;
}
mysql_free_result($request);
}
