On Tue, Nov 3, 2026 at 1:47 PM Masahiko Sawada <[email protected]> wrote:
>
> Hi,
>
> On Mon, Nov 2, 2026 at 3:17 AM Amit Kapila <[email protected]> wrote:
> >
> > On Sun, Nov 1, 2026 at 8:17 PM shihao zhong <[email protected]> wrote:
> > >
> > > Thanks for addressing all of them.
> > >
> > > Lgtm
> > >
> >
> > Thanks, here is a combined patch after fixing cosmetic issues
> > including combining one test case with an existing test. I am planning
> > to commit this tomorrow unless you or others have more comments on it.
> > Sawada-san, the issue related to autovacuum cancellation during
> > truncation raised by you was also addressed, please do let me know if
> > you need more time to review?
>
> I've reviewed the v88 patch and have some comments and questions:
>
> In lock_conflictlog_relation(), LookupGXactBySubid() doesn't check
> whether the prepared transaction actually holds a lock on the CLT, so
> there are false positives. For example, when autovacuum is truncating
> the CLT and the apply worker attempts to acquire a lock on it, the
> apply worker raises an error even if the prepared transaction created
> by the subscription doesn't hold any lock on the CLT. Since the check
> is not precise, I think the subscription could error out easily. For
> example, with disable_on_error = true, users would need to re-enable
> the subscription frequently, which doesn't seem good.
I agree with the side-effect of these false-positive cases. But adding
lock-acquisition checks could complicate the code further. I think we
can simply drop the fast-error block in lock_conflictlog_relation():
if (LookupGXactBySubid(MySubscription->oid))
ereport(ERROR, ... "could not obtain lock on conflict log table ..." ...);
and let the worker fall through to the bounded wait (lock_timeout =
deadlock_timeout + 5s) in all cases. Wouldn't that resolve the
problem?
IMO, this should eliminate the false positive where a blocking
autovacuum is mistaken for a prepared transaction. A regular
autovacuum should yield or be cancelled within that window, allowing
the worker to acquire the lock and continue. A genuine prepared
transaction, on the other hand, will continue holding the lock,
causing only that case to reach the timeout and error. Thoughts?
> ---
> Besides prepared transactions actually created by the apply worker,
> IsTwoPhaseTransactionGidForSubid() checks only the GID format, so any
> user can create a prepared transaction named pg_gid_<subid>_<xid>,
> even an empty one. This makes the apply worker of a two_phase
> subscription error out more easily, and it can also block DROP
> SUBSCRIPTION and ALTER SUBSCRIPTION ... SET (conflict_log_destination
> = 'log').
>
> ---
> ALTER SUBSCRIPTION ... OWNER TO is allowed even when the subscription
> has prepared transactions. It waits for the lock held by the prepared
> transaction, and the next conflict makes the apply worker error out.
> Should we prohibit it as we do for DROP SUBSCRIPTION and changing
> conflict_log_destination to 'log'?
>
> ---
> Is there any way to carry over CLT data across major versions?
>
> ---
> The xid in local_conflicts is a 32-bit xid. I think it's better to
> record a FullTransactionId instead. Otherwise, after xid wraparound,
> two different local transactions can be recorded with the same xid,
> which would easily confuse monitoring queries. We can't do the same
> for remote_xid since the protocol sends only a 32-bit xid, but it
> might be worth documenting.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
