Re: sequencesync worker race with REFRESH SEQUENCES

On Wed, Jul 15, 2026 at 8:28 AM Noah Misch  wrote:
>
> On Mon, Jul 13, 2026 at 03:37:54PM +0530, Amit Kapila wrote:
> > On Fri, Jul 10, 2026 at 10:22 AM Noah Misch  wrote:
> > > Fable 5 also wrote a lot more that neither it nor I confirmed by test case
> > > construction.  I'm attaching the report; feel free to disregard.  Finding-2
> > > about default_transaction_read_only=on looks worth fixing if true,
> >
> > Agreed on Finding-2  as well. The issue is that the sequencesync
> > worker sets the value via SetSequence(), which calls
> > PreventCommandIfReadOnly("setval()") for non-temp sequences, so with
> > "default_transaction_read_only=on" on the subscriber the worker's
> > transaction is read-only and sequence sync fails and never reaches
> > READY. Table apply is unaffected only because
> > ExecSimpleRelationInsert() bypasses the executor's
> > ExecCheckXactReadOnly() path which is an undocumented, untested detail
> > rather than a stated guarantee.
> >
> > For a minimal backpatch, we can force the sequencesync worker to run
> > read-write (e.g. set default_transaction_read_only=off for its session
> > at startup) so it matches table apply, plus a test that sets the GUC
> > on the subscriber and verifies sequences reach READY. Separately, it's
> > worth documenting that logical replication apply is exempt from
> > default_transaction_read_only — it's a per-transaction default meant
> > to guard user writes and never makes the node physically read-only —
> > and making that exemption explicit for all logical replication workers
> > so tables no longer rely on the bypass. What do you think?
>
> I wouldn't document those things.  default_transaction_read_only just has the
> user write "BEGIN READ WRITE" instead of plain "BEGIN".  Hence, it's more like
> an "are you sure?" prompt than a restrictive guard.  It's no surprise that
> logical replication apply achieves the equivalent of BEGIN READ WRITE; I don't
> see that outcome as an exemption.
>
> If easy, I would have the worker do the C equivalent of "BEGIN READ WRITE"
> instead of actually changing the GUC.  That makes it clear exactly which areas
> are overriding the default.  But changing the GUC is fine.
>

Fair enough. I think this means we need to set XactReadOnly as false
each time after StartTransactionCommand() (where required) as we are
doing in snapbuild.c. There seems to be multiple places and some care
is required unless we want to do it each time after
StartTransactionCommand(). So, I prefer the GUC approach and we are
already overriding it for session_replication_role and search_path
GUC's. The one minor difference could be that for PG19, we set it only
for sequencesync worker and in HEAD for all workers.

-- 
With Regards,
Amit Kapila.