Re: Proposal: Conflict log history table for Logical Replication

Hi,

I looked at v89 of the patch, I had just one small comment (different
from the comments posted already),

In insert_conflict_log_tuple(): we have the following lines

+ if (remote_commit_ts > 0)
+ values[attno++] = TimestampTzGetDatum(remote_commit_ts);
+ else
+ nulls[attno++] = true;

I think "if (remote_commit_ts != 0)" would be better for the following reasons,

1. build_local_conflicts_json_array() just above checks "if (conflicttuple->ts)"
2. The patch also has the line,

+ TimestampTz commit_ts; /* remote commit ts; 0 if unknown */

we explicitly define 0 as the sentinel when the remote commit
timestamp is unknown

Thank you,
Narayanan

On Tue, Nov 3, 2026 at 4:16 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 3, 2026 at 3:17 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Tue, Nov 3, 2026 at 1:47 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Nov 2, 2026 at 3:17 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Sun, Nov 1, 2026 at 8:17 PM shihao zhong <zhong950419(at)gmail(dot)com> 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.
> >
>
> Yes, I also thought about it previously and felt the same. To achieve
> this we need to scan the lock table for holders on the relation and
> match them to the subscription's prepared transactions (their dummy
> PGPROCs). See attached top-up atop v88 generated by AI. I can
> review/test it more if we decide to go with this strategy. I still
> feel we shouldn't add this because such scenarios will be rare in
> practice. It won't be difficult to address this if we really see such
> field reports, so I would like to leave this code as is but if you
> still think otherwise then we can take care with finding whether
> prepared xact has locked CLT.
>
> 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?
> >
>
> Yeah, this sounds better than the above idea shared by me. But this
> also won't be precise in telling the reason in case it is really
> blocked by prepared xact, OTOH, maybe we don't need to care about the
> same. Let us see what others think.
>
> > > ---
> > > 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').
> > >
>
> Right but do we really need to worry about such cases? For example,
> one can say the same about replication slots, say a user has created a
> slot with name pg_xxx as generated internally by Create Subscription
> then even Create Subscription can fail. I didn't notice such reports,
> so I'm not sure if we should try to take care of this.
>
> > > ---
> > > 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'?
> > >
>
> DROP SUBSCRIPTION and ALTER SUBSCRIPTION behaves differently because
> they will prevent new workers from starting, so there won't be a
> chance that a COMMIT PREPARED is applied and the dead lock is
> released. But for the ALTER SUBSCRIPTION ... OWNER case, the same
> issue won't happen, so it's more similar to the TRUNCATE command, and
> we didn't added checks but we can add one if you think that is really
> worth it. Again here there is a chance of false positive due to
> unrelated Prepared Transactions unless we follow something like
> attached nocfbot-v88-check_clt_locked_by_prepared_xact.
>
> > > ---
> > > Is there any way to carry over CLT data across major versions?
> > >
>
> Yes, there is a patch for this which is under discussion earlier, see
> [1] but we thought we can deal with insertion as a pre-step and then
> review that more care fully.
>
> > > ---
> > > 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.
> > >
>
> Agreed.
>
> [1]: https://www.postgresql.org/message-id/CAD21AoBNTnF6Xzo-UqGxALNqyy85EFsPWDRuXGk0aibD2ugkgQ%40mail.gmail.com
>
> --
> With Regards,
> Amit Kapila.