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

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.

--
With Regards,
Amit Kapila.