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? If so, how about putting an assertion like
'Assert(IsLogicalDecodingEnabled())' after calling
EnsureLogicalDecodingEnabled() where needed?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
