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.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 CreditAttribution: 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 CreditAttribution: chx commentedDoing that will abort the write and lose the comment. Not a good idea IMO.
Comment #3
chx CreditAttribution: chx commentedHere's the diff -b for easier review. Also, D7.
Comment #4
chx CreditAttribution: chx commentedWell, that made it easier for me to spot the wrong prefix setting.
Comment #6
chx CreditAttribution: chx commentedWeird. All the comment tests passed for me. Let's then see without the function removal.
Comment #12
chx CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: chx commentedYes.
Comment #16
webchickTagging then.
Comment #17
bdragon CreditAttribution: bdragon commentedMore tagging -- this is important for the drupal.org d7 upgrade.
Comment #18
Senpai CreditAttribution: Senpai commentedAssigning to @bdragon for review of @chx's patch in #15
Comment #19
bdragon CreditAttribution: 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 CreditAttribution: chx commented