Missing ORDER BY clause in smiley query in Subs.php

[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 Select Expand


                        // 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__);


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]

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

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.

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.

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

Given the trivial fix, let's just fix it

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.

[Arantor]

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.

[Arantor]

So I was doing some background on this, and I remembered we'd had people with issues with this in the past with the suggested code - How do I fix this "ALTER TABLE smf_smileys ORDER BY LENGTH" error I am getting? - because that's what the code actually used to be!

A bit more digging, that was to get around a limitation in MySQL 3.22 which was apparently fixed in 3.23 sometime... how times have changed. Yeah, while I'm not keen on the performance aspects, there really is no major reason now (I was sure we'd had issues with it before, but needed to find them!) against implementing this.


ETA: This is not a huge change but it's enough that I don't really want to push it into 2.0.x unless it becomes apparent that it's a really huge issue. It is, however, fixed in 2.1 with the full fixes for that documented in https://github.com/SimpleMachines/SMF2.1/pull/1128 since there's more to it than just that query - if you're not needing it to be sorted at the table level, don't sort it.