Hi Andrey, Since this serializability violation is easy to hit under load, I propose we go with your fix, which compares each summarized lock's commitSeqNo against the writer's lastCommitBeforeSnapshot. I think it's a good fix. It uses information the summarized locks already carry, needs no shared-memory layout change, and can be back-patched to all supported branches. As we discussed in a previous email, there is 1 case it does not handle: when a Serializable transaction imports a snapshot with SET TRANSACTION SNAPSHOT. For this to happen, all of the following must hold: - the transaction is read-write serializable and imports its snapshot from another serializable transaction; - the SERIALIZABLEXACT pool is exhausted, so a reader is summarized; - the summarized reader's commit falls in the window between the export and the import. I think that the above combination of events is rare enough that it shouldn't hold up this fix for PG 19. We could patch it later if we think it is important with my fix.
Attachment 0001 is your original fix, 0002 is a fix that handles the imported-snapshot case, 0003 is the injection-point spec you added, 0004 is a TAP test that fails with 0001 alone and passes with 0001+0002. We could go ahead with 0001+0003. The email thread where you proposed your patch: https://postgr.es/m/CA%2BCOZaCtK%3DUQbeQwdAoRw27J%2B58bJBC%2ByNHP4OH2%2By_t2UtFAg%40mail.gmail.com Thanks, Vaijayanti On Wed, Sep 2, 2026 at 9:01 PM Vaijayanti Bharadwaj < [email protected]> wrote: > Hi Andrey, > > After looking into this further, I realized that to solve the issue you > raised regarding imported snapshots, we can look for the actual sxact that > exported the snapshot. We can then use its lastCommitBeforeSnapshot as the > importer's lastCommitBeforeSnapshot, provided the importing transaction is > read-write. > > If the importer is read-only, a higher value does not matter and would > only result in false positives or delayed resource reclamation. For a > read-write importer, using the exporter's value vs. > PredXact->LastSxactCommitSeqNo only matters with your patch to fix the > consistency issue, as it could otherwise cause missed conflicts. > > In other words, combining the attached patch with yours resolves the > inconsistency. Without your patch, the attached patch makes no difference. > > Please let me know if you agree. I have attached the patch to illustrate > this approach. I am still testing this specific scenario and will share the > tests in a subsequent patch. > > Regards, > Vaijayanti Bharadwaj > > > On Mon, Aug 31, 2026 at 3:35 PM Vaijayanti Bharadwaj < > [email protected]> wrote: > >> My fix came from a different direction. I first hit shared memory >> exhaustion from summarized predicate locks never being freed, and while >> fixing that I considered adding finishedBefore to PREDICATELOCK. Once I had >> the field, I noticed the check in CheckTargetForConflictsIn() can use >> finishedBefore which is not populated for OldCommittedSxact, so I reused it >> for both. >> >> But you have a very good point about imported snapshots. >> GetSerializableTransactionSnapshotInt() sets SeqNo.lastCommitBeforeSnapshot >> from the live PredXact->LastSxactCommitSeqNo at import time, while >> sxact->xmin comes from the imported snapshot, and nothing reconciles the >> two. lastCommitBeforeSnapshot can end up too large. Like you said, a reader >> that commits between export and import loses its rw-edge. That is a missed >> conflict rather than a false positive. >> >> I think its a super rare case, it needs a read-write SERIALIZABLE >> transaction importing a snapshot, pg_dump uses REPEATABLE READ, READ ONLY >> for any connection that imports, and logical replication tablesync is also >> REPEATABLE READ. But an application can do it. >> >> I think your approach would work for the above case with a small >> addition: flag sxacts that imported a snapshot, and in >> PredicateLockIsForOverlappingTransaction() treat every summarized lock as >> overlapping for those. It is pessimistic, but only for a rare case. >> >> So far, I have only looked into >> PredicateLockIsForOverlappingTransaction(), and have yet to evaluate other >> uses of lastCommitBeforeSnapshot for the case of importing a snapshot. But >> if you have done so, I would like to understand that. >> >> Thank you! >> Vaijayanti Bharadwaj >> >> On Mon, Aug 31, 2026 at 1:37 PM Andrey Borodin <[email protected]> >> wrote: >> >>> Hi Vaijayanti, >>> >>> Jacob reported what looks like the same bug earlier [0], and I sent a >>> tentative commitSeqNo-based fix in that thread. >>> >>> Your patch made me notice a case which we did not consider there: >>> imported >>> serializable snapshots. lastCommitBeforeSnapshot is recorded when the >>> snapshot is imported, while finishedBefore is compared with the imported >>> snapshot's xmin. The two approaches can therefore disagree about a >>> reader >>> which commits between export and import. Your patch preserves the >>> existing >>> non-summarized behavior in this case; mine does not. >>> >>> Was this case part of your reasoning? It seems worth writing explicitly >>> the >>> expected behavior here. >>> >>> Thank you! >>> >>> Best regards, Andrey Borodin. >>> >>> >>> [0] >>> https://postgr.es/m/CA%2BCOZaCtK%3DUQbeQwdAoRw27J%2B58bJBC%2ByNHP4OH2%2By_t2UtFAg%40mail.gmail.com >>> >>>
0002-Fix-consistency-issue-when-importing-a-serializable-.patch
Description: Binary data
0003-Test-SSI-conflict-detection-after-summarization.patch
Description: Binary data
0001-Detect-SSI-conflicts-with-summarized-predicate-locks.patch
Description: Binary data
0004-Test-SSI-conflict-detection-for-imported-snapshots-a.patch
Description: Binary data
