Feedback-ID: ie3de48e3:Fastmail
Date: Wed, 9 Sep 2026 12:20:22 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Antonin Houska <ah@cybertec.at>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 Mihail Nikalayeu <mihailnikalayeu@gmail.com>,
	Andres Freund <andres@anarazel.de>
Subject: Re: Race conditions in logical decoding
Message-ID: <aqEtEZP-7kpX3_Ov@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <TY4PR01MB177187B7CB7BE12F70F806BE494A22@TY4PR01MB17718.jpnprd01.prod.outlook.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aqEtEZP-7kpX3_Ov%40alvherre.pgsql>
Precedence: bulk

Hello, replying to Hou and Houska emails in one.

On 2026-Aug-22, Zhijie Hou (Fujitsu) wrote:

> I think the cache might be better placed in the SnapBuild struct (at least on
> HEAD) rather than in static variables. As currently written, it persists across
> decoding sessions in the same backend - a session could build a snapshot, drop
> the slot, and later create a new slot and build another snapshot, potentially
> consulting stale entries from the first builder. For example, it has a wraparound
> concern: after XID wrap, a cached value could refer to a different transaction,
> causing us to skip the CLOG wait and reintroduce the inconsistency this patch
> aims to fix.

Hmm, yeah, there's definitely a problem with that.  Now, this bug can
affect logical decoding in existing releases as well, so we should
backpatch this fix, and I'm not sure about changing SnapBuild in
backbranches.  Maybe another approach is to use file-level statics
(rather than function-level) so that they can be reset by slot drop
routines.

> Besides, just to confirm one note: IIUC, for exported snapshots by logicalrep, a
> transaction could be treated as committed while still in PGPROC, while
> concurrent MVCC snapshots still see it as in progress which looks inconsistent.
> I understand that waiting for ProcArray removal in the general case could
> deadlock against synchronous replication, so it's probably acceptable to leave
> it unchanged for internal usage in active replication processes.

OK.  TBH I'm somewhat unease about this inconsistency; I wondered about
doing the CLOG-based test only in sync replication and using
XidIsInProgress otherwise, but didn't really try (which is to say: I'm
not even sure if it's _possible_ at all.)

> However, for cases where the snapshot is exported, would it be possible to
> additionally wait for it in SnapBuildInitialSnapshot() (which is used only by
> CREATE_REPLICATION_SLOT and REPACK)? Since that runs before START_REPLICATION,
> the process isn't streaming or feeding any subscriber, so I believe the deadlock
> wouldn't occur there. (I think that the walsender executing
> CREATE_REPLICATION_SLOT shouldn't be added to sync_standby_names, otherwise
> building the initial snapshot itself would already have a deadlock risk via
> SnapBuildWaitSnapshot->XactLockTableWait.)

Yeah, we could do that.

Do you want to try and write a patch?


On 2026-Aug-25, Antonin Houska wrote:

> 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.

I think we should absolutely do something like that, because repeated
list_member_oid() are unlikely to be great.  Maybe as output of each run
we end up with an unsorted array; when SnapBuildBuildSnapshot runs next
time, the first thing we do is sort the array for bsearch.  That way, we
don't have to sort unless absolutely necessary.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Para tener más hay que desear menos"