Date: Wed, 15 Jul 2026 04:56:01 -0700
From: Noah Misch <noah@leadboat.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: vignesh21@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: sequencesync worker race with REFRESH SEQUENCES
Message-ID: <20260715115601.fd.noahmisch@microsoft.com>
References: <20260710045217.f0.noahmisch@microsoft.com>
 <CAA4eK1K8LD243UzHgVNCm4skJZ4UCjR3vowDhKp=cWnK5oBT-Q@mail.gmail.com>
 <20260715025809.cd.noahmisch@microsoft.com>
 <CAA4eK1+k+iKhNtt6bq2SJQm+K_ZYUrS5GCqnf6a6qK6_DrX5Gg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAA4eK1+k+iKhNtt6bq2SJQm+K_ZYUrS5GCqnf6a6qK6_DrX5Gg@mail.gmail.com>
User-Agent: Mutt/2.3.0 (2026-01-25)
Archived-At: 
 <https://www.postgresql.org/message-id/20260715115601.fd.noahmisch%40microsoft.com>
Precedence: bulk

On Wed, Jul 15, 2026 at 05:23:05PM +0530, Amit Kapila wrote:
> On Wed, Jul 15, 2026 at 8:28 AM Noah Misch <noah@leadboat.com> 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 <noah@leadboat.com> 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

In that case, changing the GUC works for me.