Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Apache/2.4.18 (Ubuntu)
PHP Version 7.0.8 ubuntu 16.04.2 x64
1. Create content type
2. Add some records
3. Add a comment field to the content type
4. Create view from that content type
5. Once I add the field Comment count to the view, the records disappear
Records should not disappear. Looks like it is SQL problem it is with INNER join to table "comment_entity_statistics", should be with LEFT join.
SQL from my environment
SELECT node_field_data.langcode AS node_field_data_langcode, comment_entity_statistics.comment_count AS comment_entity_statistics_comment_count, node_field_data.created AS node_field_data_created, node_field_data.nid AS nid
FROM
{node_field_data} node_field_data
INNER JOIN {comment_entity_statistics} comment_entity_statistics ON node_field_data.nid = comment_entity_statistics.entity_id AND comment_entity_statistics.entity_type = 'node'
WHERE (( (node_field_data.status = '1') AND (node_field_data.type IN ('raksts')) ))
ORDER BY node_field_data_created DESC
LIMIT 10 OFFSET 0
I fixed it by adding records in table "comment_entity_statistics" manually.
Proposed resolution
Change the INNER join to a LEFT join. This effectively reverts #375342: Unnecessary LEFT JOIN on {node_comment_statistics} causing bad query performance which was done in the days of D6 when comments weren't a field yet. That commit may have given performance and made no harm then, but together with other changes now it is a performance optimisation that does harm.
Remaining tasks
Review
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 2801929-30.patch | 9.57 KB | Lendude |
| #32 | 2801929-30-TEST-ONLY.patch | 8.88 KB | Lendude |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan commentedI helped @kristaps.vitolins to investigate this issue and come to the same conclusion. Records in the 'comment_entity_statistics' table are only added after a comment field was added to the entity.
Comment #3
Sutharsan CreditAttribution: Sutharsan commentedThis patches fixes it. But it considering the comments in the code about multiple comment fields in one entity, I expect some complexities and this fix may have side effects.
Comment #4
Sutharsan CreditAttribution: Sutharsan commentedComment #5
dawehnerOne thing I'd certainly check with git blame in core and the old views repo, whether in the past this used to be LEFT and went to INNER to fix some bug.
Comment #6
Sutharsan CreditAttribution: Sutharsan commentedIt has been 'INNER' since the early days of Views 7.x-3.x (31dd0540) November 2009. That is as far as I traced it back, did not look any further (yet).
Comment #10
andypostClosed duplicate #2870607: Views 'comment count' field doesn't display on referenced node with 0 comment thru relationships
Comment #11
dawehnerI just had a look, this would effectively revert #375342: Unnecessary LEFT JOIN on {node_comment_statistics} causing bad query performance. It was just used for performance reasons originally, but well, the original assumption was wrong. It is not a 1to1 relationship.
Is there a way how we could test it?
Comment #12
gilles.koffmann CreditAttribution: gilles.koffmann commentedHere is the patch for 8.5.3... But it does not seem to resolve the problem on this drupal version...
Comment #13
andypostComment #15
reszlitested on 8.5.5 and it works as expected
Comment #16
andypostIt still needs test and approval why it reverts #375342: Unnecessary LEFT JOIN on {node_comment_statistics} causing bad query performance
Comment #17
nashkrammer CreditAttribution: nashkrammer commentedTested patch on #12 8.5.8 core and works as expected.
Comment #18
DamienGR CreditAttribution: DamienGR as a volunteer commentedTested on 8.6.7 and works great
Comment #21
geek-merlinClosed #2654986: A views may not display all content if we use the "comment_count" field and the field was added after some content exists as dup of this.
As of #16:
> It still needs ... approval why it reverts #375342: Unnecessary LEFT JOIN on {node_comment_statistics} causing bad query performance
This is answered by @dawehner #11 (and who should know better than them): This was originally intented to be a performance optimization with no result change based on the assumption "{node_comment_statistics} is always 1:1 on {node}" (IS). That turned out to be wrong and results do change. So that commit was wrong.
Additionally, #2330627-8: Create CommentStatistics entries when new comment field is created. does not provide any reason that comment statistics are to be created mandatorily
So NW for tests only.
Comment #22
LendudeThe original fix was for D6 and I think this was true then since comments weren't a field back in those days.
Comment #23
geek-merlin> The original fix was for D6 and I think this was true then
I suppose you are right. So let's re-state: That commit may have given performance and made no harm then, but together with other changes now it is a performance optimization that does harm.
Comment #24
Lendude#23 sounds spot on, updated the IS to reflect the direction of the change and the discussion about reverting an old optimisation.
Comment #26
wombatbuddy CreditAttribution: wombatbuddy commentedTested patch #12 on 8.8.4 core and works as expected.
Comment #27
LendudeHere is a test for this.
Interdiff is the test only file.
Comment #29
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedComment #30
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixed the deprecated function.
Comment #31
geek-merlin@ridhimaabrol24 Can you post the new test-only to validate it is red so provokes the issue?
Comment #32
LendudeHere we go
Comment #33
LendudeAh sorry forgot to add the original patch, we want de last patch to be the one with the fix
Comment #35
geek-merlinCode looks good. Test provokes the issue and that oneliner fixes it.
Comment #36
JordiK CreditAttribution: JordiK commentedApplied cleanly and worked for me on D8.9.
Comment #37
Rohit Tiwari CreditAttribution: Rohit Tiwari at Srijan | A Material+ Company for Drupal India Association commentedComment #39
andypostbot flux
Comment #41
LendudeUnrelated fail
Comment #42
alexpottCommitted and pushed a76a7586a7 to 9.1.x and 08b33b5731 to 9.0.x and c68ebfbc41 to 8.9.x. Thanks!
Ran the test locally on 8.9.x and it's green so all good for a backport.