SMF Development > Fixed or Bogus Bugs

Missing ORDER BY clause in smiley query in Subs.php

(1/2) > >>

Riven8192:
The rolleyes smiley came up as ':' + ':)' instead of '::)', so I looked in the SMF code that produced the smileys.

Please note how the PHP comment states the smileys are fetched in reverse order, while this is not in the SQL query.


--- Code: ---                        // Load the smileys in reverse order by length so they don't get parsed wrong.
                        if (($temp = cache_get_data('parsing_smileys', 480)) == null)
                        {
                                $result = db_query("
                                       SELECT code, filename, description
                                       FROM {$db_prefix}smileys
                                       ----------> ORDER BY LENGTH(code) DESC <-------------
                               ", __FILE__, __LINE__);

--- End code ---

This one-line fix did the trick!

emanuele:
Hello Riven8192 and welcome to sm.org!

Thanks for the report, however I've to say that usually this shouldn't be necessary because the order of the smiley is set altering the table in the management page...unless it could be the same problem that from time to time occurs with boards order... (was it related to myisam vs innodb? don't remember...)

Riven8192:
Please note that the order of INSERTs it not necessarily the order in which the data is stored on disk.

If ORDER BY is not explicitly specified in a SELECT query, NO assumptions should be made on the order of rows in the resultset.

I'm stressing this, because obviously this broke the smileys on my forum, which simply means it happens.

Given the trivial fix, let's just fix it :)

emanuele:

--- Quote from: Riven8192 on April 08, 2012, 12:16:52 PM ---Please note that the order of INSERTs it not necessarily the order in which the data is stored on disk.
--- End quote ---
I know, they are not sorted by insert, the code in managesmileys alters the table cased on the length of the code as it should be. ;)


--- Quote from: Riven8192 on April 08, 2012, 12:16:52 PM ---If ORDER BY is not explicitly specified in a SELECT query, NO assumptions should be made on the order of rows in the resultset.

I'm stressing this, because obviously this broke the smileys on my forum, which simply means it happens.
--- End quote ---
SMF already does this assumption in at least two places: one is the smiley and the second is the boards order (if you check the code boards are retrieved without any sort, just relying on MyISAM ability to keep and provide the correct order).


--- Quote from: Riven8192 on April 08, 2012, 12:16:52 PM ---Given the trivial fix, let's just fix it :)

--- End quote ---
Given the fact that this (probably) was done like that for a reason (don't ask me which reason, I can assume it was optimization due to the fact that the order *should* be 99.99% +/- 0.01% of the times correct), I would wait for someone with more knowledge than me on the actual reason before do such changes. ;)

:
Well, you could perform an ALTER TABLE ... ORDER BY and it *would* force the order physically on disk where you could consistently rely on it.

The point of this is actually performance. If you make sure the table is in the correct order when you actually update it, reading the table requires precisely zero overhead for sorting because you know it's already in the right order. That's why it's done, it's infrequently modified, to the point where it's essentially static data and you might as well pull a little bit of performance for that to effectively ORDER BY null because you "don't care" about the order, with the assertion that the order will be correct.

Unfortunately with InnoDB and also with MyISAM on 5.6+, this is not an assertion we can rely on any more. The board index will require a rewrite to accommodate this without a serious performance hit (since in the board index's case, that would force a filesort on the ordering)

Here... that's actually a similar problem for a different reason. MySQL will always have a filesort when you're doing the ordering on a tinytext/text/mediumtext etc (which is the board index's problem)... but doing an EXPLAIN on the above query does show it adds a filesort there too. There's the core reason for not doing it - performance.

Now, there's no way to optimise this with an index - even if you go to the fun of adding a new column and then create an index on that, it will not be able to optimise the query for MySQL.

Given that it's cached, I'm inclined to go with the suggested fix - and while at it, to drop the alter table - but I'm not sure I see it as the ideal.

Navigation

[0] Message Index

[#] Next page

Go to full version