Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
This bug seems to have been introduced in #292984: Add cid to node_comment_statistics.
Instead of showing recent comments, it shows all comments (no matter how old) for the most recently-commented-on node, then the next newest node, and so on.
Comment | File | Size | Author |
---|---|---|---|
#13 | Screen Shot 2011-11-29 at 10.44.22 AM.png | 10.79 KB | grendzy |
#12 | Screen Shot 2011-11-29 at 07.45.59 .png | 46.43 KB | msonnabaum |
#12 | Screen Shot 2011-11-29 at 07.45.19 .png | 43.88 KB | msonnabaum |
#5 | comment_order.patch | 1.63 KB | grendzy |
#4 | comment_order-D8.patch | 1.63 KB | grendzy |
Comments
Comment #1
sunThis is the current query:
Actually, upon looking at this again, I've no idea why this is joining on node_comment_statistics at all.
Someone should run the patched query against an SQL EXPLAIN. We might need an index on comment.created.
Comment #2
sunComment #3
grendzy CreditAttribution: grendzy commentedHere's the explain: I agree on both points, we should have an index, and I can't find a reason to join node_comment_statistics.
Comment #4
grendzy CreditAttribution: grendzy commentedComment #5
grendzy CreditAttribution: grendzy commentedgo go gadget testbot!
Comment #6
sunLooks ready to fly for both D8 and D7 to me.
Comment #7
catchComment #8
Dries CreditAttribution: Dries commentedCommitted these patches to 7.x and 8.x, respectively. Thanks all.
Comment #10
catchThis patch is causing random test failures for both D7 and D8, not re-opening the issue, but #1188880: Comment module test fails randomly is where we're working on it.
Comment #11
catchActually I am re-opening it, because that issue is critical, but we need to discuss whether that index is really necessary or not.
Comment #12
msonnabaum CreditAttribution: msonnabaum commentedIt appears this change introduced a performance regression.
I was seeing this query take over a second, and I noticed that it hits the node table first, so it can't use comment's indexes for the sort, leading to "Using temporary; Using filesort", on some potentially very big tables.
Adding a STRAIGHT_JOIN reverses the order so that the comment indexes get used first, which removes the filesort and tmp table as well as only scans the 10 rows it needs. I just don't know how or if that's a possibility with DBTNG, so I'll need some help there.
Comment #13
grendzy CreditAttribution: grendzy commentedOdd. When I test, the filesort only occurs with #1188880: Comment module test fails randomly applied, and the results are different (the join order isn't affected, but I do get a filesort on the comment table) If I rollback #1188880 it's back to the EXPLAIN result in #4. Could this be caused by differences in the query optimizer between MariaDB and MySQL? I have 5.1.36.
Comment #14
Liam McDermott CreditAttribution: Liam McDermott commentedI just tried this on MySQL 5.0.96 and 5.1.63 and am seeing a temp table and filesort, same as #12.
This is such a bad problem on the test site I'm helping a client with the temp table grows >2GB in size before it runs out of room in the /tmp partition, then throws an error that makes the site inaccessible (after waiting several minutes for the page to load). In case anyone is interested, the site has around 160,000 nodes and 800,000 comments. Once the query is cached however, the page loads in a second or two (sorry for the lack of hard figures, but the difference is loading times of seconds versus minutes so I haven't bothered with benchmarking).
It might seem like heresy to do it this way, but this query avoids filesorts:
I'm pretty sure it's not possible with DBTNG, as it stands. That's what prompted me to start thinking of other ways of avoiding the creation of tmp tables and filesorts.
I can confirm your STRAIGHT_JOIN addition works for me, too.
Comment #15
alexpottThis is now a view
Comment #16
lizzjoyComment #17
lizzjoyI changed comments from Full to Filtered HTML since #2378211: no access to issue comments was posted.