Re: POC: enable logical decoding when wal_level = 'replica' without a server restart - Mailing list pgsql-hackers

On Thu, Oct 30, 2025 at 9:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 31, 2025 at 3:30 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Oct 29, 2025 at 8:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Oct 30, 2025 at 12:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Wed, Oct 29, 2025 at 5:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > >
> > > > > 8.
> > > > > @@ -136,6 +137,12 @@ create_logical_replication_slot(char *name, char *plugin,
> > > > >     temporary ? RS_TEMPORARY : RS_EPHEMERAL, two_phase,
> > > > >     failover, false);
> > > > >
> > > > > + /*
> > > > > + * Ensure the logical decoding is enabled before initializing the logical
> > > > > + * decoding context.
> > > > > + */
> > > > > + EnsureLogicalDecodingEnabled();
> > > > > …
> > > > >
> > > > > EnsureLogicalDecodingEnabled(void)
> > > > > {
> > > > > Assert(MyReplicationSlot);
> > > > >
> > > > > if (wal_level != WAL_LEVEL_REPLICA)
> > > > > return;
> > > > >
> > > > > /* Prepare and start the activation process if it's disabled */
> > > > > if (!start_logical_decoding_status_change(true))
> > > > > return;
> > > > >
> > > > > I see that EnsureLogicalDecodingEnabled() sometimes returns without
> > > > > enabling decoding (like when wal_level is not a replica). It is
> > > > > possible that the same is not possible in this code flow, but won't it
> > > > > be better to get the return value and assert if this function returns
> > > > > false?
> > > >
> > > > Do you mean to check the value returned by
> > > > start_logical_decoding_change()? I could not understand what assertion
> > > > we can put there.
> > > >
> > >
> > > I mean to check the return value of EnsureLogicalDecodingEnabled()
> > > because there are two code paths as quoted in my message from where if
> > > this function returned, the logical decoding won't be enabled.
> > >
> > > > Related to this point, I thought it might be better to raise an error
> > > > if this function is called when wal_level='minimal' instead of doing
> > > > nothing because it cannot ensure logical decoding enabled by calling
> > > > this function.
> > > >
> > >
> > > Yeah, that will also work for one of the code paths in
> > > EnsureLogicalDecodingEnabled() but I think we can still return without
> > > enabling the decoding from the other code path quoted in my message.
> >
> > There are four cases EnsureLogicalDecodingEnabled() returns without
> > changing the status (i.e., when start_logical_decoding_status_change()
> > returns false):
> >
> > 1. wal_level is 'minimal'.
> > 2. wal_level is 'logical'.
> > 3. wal_level is 'replica', but during recovery (the logical decoding
> > status depends on
> > 4. wal_level is 'replica', but logical decoding is already enabled.
> >
> > Among these four cases, case 1 is the case where logical decoding is
> > not available even though we call EnsureLogicalDecodingEnabled().
> > Also, in the case 3, the function could just return and logical
> > decoding is not enabled if the primary doesn't enable logical
> > decoding.
> >
> > The code you quoted is the change in
> > create_logical_replication_slot(), and the both cases don't happen
> > there since we already passed CheckLogicalDecodingRequirements()
> > check. IIUC your comment is meant to check if logical decoding is
> > really enabled after calling EnsureLogicalDecodingEnabled(). Is that
> > right?
> >
>
> Yes.
>
> > If so, how about putting an assertion like
> > 'Assert(IsLogicalDecodingEnabled())' after calling
> > EnsureLogicalDecodingEnabled() where needed?
> >
>
> WFM.

Great.

I've updated the patch based on the comments I got so far, and updated
the commit message too. Please review it.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com