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").
| Comment | File | Size | Author |
|---|---|---|---|
| bud-join.patch | 696 bytes | simon georges |
Comments
Comment #1
simon georges commentedThis patch is part of the #1day1patch initiative.
Comment #2
simon georges commentedChanging typo in title (sigh...).
Comment #3
polOh 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.
Comment #4
simon georges commentedI'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!
Comment #5
polHello 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.
Comment #6
simon georges commentedOk, 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?
Comment #7
simon georges commentedNow 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...
Comment #8
polHello 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.
Comment #9
simon georges commentedNothing 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!