Hi,

I'm just wondering why you use two joins instead of just one. With your code, the EXPLAIN gives :

+----+-------------+-------+-------+------------------+---------+---------+-------------------+------+----------------+
| id | select_type | table | type  | possible_keys    | key     | key_len | ref               | rows | Extra          |
+----+-------------+-------+-------+------------------+---------+---------+-------------------+------+----------------+
|  1 | SIMPLE      | b2    | const | PRIMARY,tmd,list | PRIMARY | 4       | const             |    1 | Using filesort |
|  1 | SIMPLE      | b3    | const | PRIMARY          | PRIMARY | 4       | const             |    1 |                |
|  1 | SIMPLE      | b1    | ref   | tmd,list         | list    | 389     | const,const,const |    2 | Using where    |
+----+-------------+-------+-------+------------------+---------+---------+-------------------+------+----------------+

, with mine :

+----+-------------+-------+-------+------------------+---------+---------+-------------------+------+----------------+
| id | select_type | table | type  | possible_keys    | key     | key_len | ref               | rows | Extra          |
+----+-------------+-------+-------+------------------+---------+---------+-------------------+------+----------------+
|  1 | SIMPLE      | b2    | const | PRIMARY,tmd,list | PRIMARY | 4       | const             |    1 | Using filesort |
|  1 | SIMPLE      | b1    | ref   | tmd,list         | list    | 389     | const,const,const |    2 | Using where    |
+----+-------------+-------+-------+------------------+---------+---------+-------------------+------+----------------+

.

The question is... is it faster or not ;-)

Patch attached (the "innerJoin" is just cosmetic, it avoids me to wonder if it's an inner or an outer when I read the code, but it's totally the same as a simple "join").

CommentFileSizeAuthor
bud-join.patch696 bytessimon georges

Comments

simon georges’s picture

This patch is part of the #1day1patch initiative.

simon georges’s picture

Title: Performance imporovement (?) » Performance improvement (?)

Changing typo in title (sigh...).

pol’s picture

Status: Needs review » Fixed

Oh that's great, I didn't knew that.

Patch committed, thanks again !

Edit: Looks like drush am didn't work correctly and forgot to use your credentials for the patch. Sorry mate.

simon georges’s picture

I'll survive ;-) You're quick, as ever.
After a careful examination, I don't think I have any patch left for the module, it's perfect as it is, great job!

pol’s picture

Status: Fixed » Needs work

Hello Simon,

I had to revert the patch.

I remember now that I had the problem, and that's why I did it using two joins.
If you apply your patch and 'up' the first block of a region, it will disappear. Same if you 'down' the last block.

simon georges’s picture

Ok, that's really strange, because the SQL query seems to return stricly the same results, and it worked when I tested it with 2 blocks.
So I did a few tests with a lot of blocks on a page. And I saw the issue. I swapped to the old code, the issue disappeared. I added the new code again... and the issue was gone.
The only thing possible is when the ajax fires, the CSS identifiers are wrong and the JS "detaches" the block without being able to attach it after. The thing is, it should happen with your code too since the SQL results are exactly the same...

There's clearly a bug somewhere, but I'm not being able to pinpoint it at the moment, since it works. Wouldn't it be a classic "clear cache WTF" issue somewhere?

simon georges’s picture

Now that I think of it, it could happen if you put in the region blocks you're user cannot see because of a permission. If the blocks is at the wrong position, the up won't do anything except remove the block from the page. But I think it appears with your old code too.
And it's actually a bug ;-) It disappeared because as I clicked "Up" on all the blocks to test it, they all went up above the "missing" block, and I couldn't reproduce it after...

pol’s picture

Hello Simon,

I don't remember why I had to do like that, but I remember there was something particular about it and SQL results were differents.

And about the permission stuff, this is indeed something that I hadn't think yet, good job, this is something to do.

simon georges’s picture

Status: Needs work » Closed (won't fix)

Nothing to do anymore since the code has been reworked in #2000732: Removes block when we click move up link due to invisible blocks in the same region.. Good job!