While analyzing issues on drupal.org, I detected that it is possible for multiple comments on a node to have the same {comment}.thread value.
To ensure data integrity, we should have a unique index on (nid, thread).
Drupal.org had 34 instances where duplication had happened. Most of this apparently happened at times where things were especially flaky. (I manually repaired the damage already.)
Detecting duplicate thread values:
(d6 naming, use 'comment' for a d7+ db)
select distinct l.nid from comments l, comments r WHERE l.nid = r.nid and l.thread = r.thread and l.cid < r.cid;
This takes 5 minutes to run on drupal.org slave.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 1685170_15.patch | 4.62 KB | chx |
| #13 | tsung.xml_.txt | 2.29 KB | bdragon |
| #12 | 1685170_7.patch | 7.21 KB | chx |
| #6 | 1685170_6.patch | 4.91 KB | chx |
| #4 | 1685170_4-d7-do-not-test.patch | 6.15 KB | chx |
Comments
Comment #1
bdragon commentedHere's a patch to add the unique key.
It's probably faster to attempt to add the key and error out than it is to search for problems.
Whether this is acceptable or not, I don't know.
Comment #2
chx commentedDoing that will abort the write and lose the comment. Not a good idea IMO.
Comment #3
chx commentedHere's the diff -b for easier review. Also, D7.
Comment #4
chx commentedWell, that made it easier for me to spot the wrong prefix setting.
Comment #6
chx commentedWeird. All the comment tests passed for me. Let's then see without the function removal.
Comment #12
chx commentedWe had a bunch of bot flukes but we are seemingly back on track. So here's a much more complete version, with lots more comment, lock_release and all that.
Comment #13
bdragon commentedTested against postgresql and mysql using tsung.
Here's the configuration I used.
With the patch in I was unable to cause a duplicate.
Without the patch, I was getting a large percentage of duplicates -- some were even 3-way dupes.
Comment #14
webchickThis felt like something we should add tests for, but chx explained that it's catching a race condition and we don't have a good way for testing for those. Thanks for the pointer to Tsung; never heard of that before!
Anyway, this looks fairly sane. It's adding checks for locking around this code and failing if they're encountered.
Committed and pushed to 8.x. Not sure if we should backport this to D7 too?
Comment #15
chx commentedYes.
Comment #16
webchickTagging then.
Comment #17
bdragon commentedMore tagging -- this is important for the drupal.org d7 upgrade.
Comment #18
senpai commentedAssigning to @bdragon for review of @chx's patch in #15
Comment #19
bdragon commentedI'm getting duplicates during testing even after applying #15.
Also, typo in comment
+ // We need a lopp here to avoid race conditions: if another process
-- should be 'loop' instead of 'lopp'.
Comment #20
drummAdding an estimate for closing this out and/or mitigating it on Drupal.org.
Comment #21
drummThis has been mitigated for Drupal.org: #1944448: Drupal.org D7 deployment. We'll run a query to check for duplicates before proceeding.
Comment #22
chx commentedComment #28
slowflyer commentedJust had this issue today on a 10.2.6 website. The related comments have been saved in the same second and resulted in same thread value.
Comment #29
quietone commented@slowflyer, thanks for reporting the problem. It is good to know that this can still happen.
Since, It is better to open a new issue than re-open and old one. Also, 10,2,x is only receiving security fixes now. have made a new issue #3459807: Duplicate thread on the same node.
Therefor, I have made a new issue from comment #28 and referenced this issue.
I am restoring the Meta data on this issue.
Comment #30
quietone commentedAdding credit for the commit and updating issue meta.