Re: Race conditions in logical decoding

Alvaro Herrera  wrote:

> On 2026-Sep-18, Antonin Houska wrote:
> 
> > Maybe I miss the point, but what's wrong about modifying the existing loop
> > that inverts the meaning of the ->xip array
> > 
> > 	/*
> > 	 * snapbuild.c builds transactions in an "inverted" manner, which means it
> > 	 * stores committed transactions in ->xip, not ones in progress. Build a
> > 	 * classical snapshot by marking all non-committed transactions as
> > 	 * in-progress. This can be expensive.
> > 	 */
> > 	for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
> > 	{
> > 		...
> > 	}
> > 
> > by calling XactLockTableWait() for each XID we find in the array (i.e. each
> > committed transaction)?
> 
> Ah, you mean something like the attached quick POC?  This does pass the
> two tests that Rui wrote, also attached.  (I didn't test Zhijie's, which
> AFAICT is written to pass with the bug and fail without it.)

Yes, I mean checking if those transactions have really ended.

Regarding [1], perhaps the idea is to avoid calling XactLockTableWait() if the
transaction is no longer in procarray. I'm not sure it's a problem to call
that function (possibly many times) for transactions that are no longer
running. (GetRunningTransactionData() is not free either.)

And regarding the deadlock with synchronous replica, my understanding is that
we avoid it by waiting in SnapBuildInitialSnapshot() instead of in
SnapBuildBuildSnapshot().

[1] https://www.postgresql.org/message-id/CAHWVJhHXyLtS-8mdL9WhEWfsERb%3DFN7JdPD0GYAXgTmCnqbYGw%40mail.gmail.com

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com