Álvaro Herrera <[email protected]> wrote: > On 2026-Mar-20, Álvaro Herrera wrote: > > > Failing other ideas, I think we should just go with 0001. We'd need more > > commentary on why is TransactionIdDidCommit() OK, when we haven't > > scanned PGPROC for that xid, though. > > I spent some more time stepping through the motions here. In the test I > saw, the problem is caused by the check for latestCompletedXid. The > transaction we saw as committed in WAL has not yet been removed from > ProcArray, which is what updates latestCompletedXid. So that makes > TransactionIdIsInProgress() report that yes, the transaction is in > progress, therefore we continue to wait in a loop forever, at least in > synchronous replication.
> To recap: the problem was that returned a snapshot with a transaction > recorded as committed, but which was not yet marked as such in CLOG, so > when we did things like HeapTupleSatisfiesMVCC() with the snapshot so > obtained, it would run TransactionIdDidCommit(), get false from it, and > conclude that the transaction "must have aborted or crashed", therefore > marking the tuple as HEAP_XMIN_INVALID. So what we do here is ensure > that TransactionIdDidCommit() will return the correct value before > giving the snapshot back. > > > The other problem with this patch in the back of my mind was that we may > be doing TransactionIdDidCommit() potentially for a lot of transactions. > Instrumenting these code paths I saw that some tests in the suite would > call the transam.c routine several thousand times, and some XIDs would > repeat over and over. This may not sound like much, but we don't > actually know what happens in production systems; and every transam.c > call has the potential to do I/O to get the relevant CLOG page. And > because we do this snapshot building in places like > SnapBuildProcessChange(), it has the potential to do nasty. So I added > a quick and dirty process-local cache: the list of transactions we > tested on the previous cycle. We don't test nor wait for any > transaction that's on that list, since evidently we must have tested it > already and it cannot become uncommitted after that. All in all, we > test for each potentially in-progress transaction just once per backend. > > So, what do you think of the attached? I appreciate it that you performed the tests. I considered the race condition pretty rarely, however it does not imply anything about the cost of the checks: yes they can be quite frequent. I'm just thinking if the 'xids_already_tested' variable name is appropriate. Since you only add XIDs known to be committed, how about something like 'xids_known_committed'? Besides, that, it occurred to me that a sorted array might be appropriate instead of a list, so that bsearch() can be used, but I'm not sure about that. > (On second thought, it may be a good idea to plant some of my > explanation above in the new comment in SnapBuildBuildSnapshot. No time > for that right now though.) I think it's worth mentioning at least the synchronous replication problem you mentioned above, so it's easier to understand why we cannot use TransactionIdIsInProgress(): -- Antonin Houska Web: https://www.cybertec-postgresql.com
