Re: POC: enable logical decoding when wal_level = 'replica' without a server restart

On Mon, Dec 15, 2025 at 11:16 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Dec 15, 2025 at 10:32 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Sawada-San.
> >
> > Some minor review comments for v35-0001.
> >
> > ======
> > src/backend/replication/logical/logicalctl.c
> >
> > ProcessBarrierUpdateXLogLogicalInfo:
> >
> > 1.
> > +/*
> > + * This routine is called when we are told to update XLogLogicalInfo
> > + * by a ProcSignalBarrier.
> > + */
> > +bool
> > +ProcessBarrierUpdateXLogLogicalInfo(void)
> > +{
> > + if (GetTopTransactionIdIfAny() != InvalidTransactionId)
> > + {
> > + /* Delay updating XLogLogicalInfo until the transaction end */
> > + XLogLogicalInfoUpdatePending = true;
> > + }
> > + else
> > + update_xlog_logical_info();
> > +
> > + return true;
> > +}
> > +
> >
> > Strange to have a boolean function that unconditionally returns true.
> > Should the function comment explain the meaning of the (always true)
> > return value?
>
> Given it's a API contract between ProcessProcSignalBarrier() and the
> barrier-processing functions, it seems to make sense to explain what
> the returned value means in ProcessProcSignalBarrier(), and we already
> have such comments:
>
> * Process each type of barrier. The barrier-processing functions
> * should normally return true, but may return false if the
> * barrier can't be absorbed at the current time. This should be
> * rare, because it's pretty expensive. Every single
> * CHECK_FOR_INTERRUPTS() will return here until we manage to
> * absorb the barrier, and that cost will add up in a hurry.
>
> >
> > The return value is assigned to a variable which defaults true anyway,
> > so really this function might as well be void.
>
> I think it's not a good idea that ProcessProcSignalBarrier() calls
> each of the barrier-processing functions differently.
>
> >
> > ~~~
> >
> > EnsureLogicalDecodingEnabled:
> >
> > 2.
> > + if (RecoveryInProgress())
> > + {
> > + /*
> > + * CheckLogicalDecodingRequirements() must have already error out if
> > + * logical decoding is not enabled since we cannot enable the logical
> > + * decoding status during recovery.
> > + */
> > + Assert(IsLogicalDecodingEnabled());
> > + return;
> > + }
> >
> > typo? "already error out"
>
> Fixed.
>
> >
> > ======
> > .../recovery/t/050_effective_wal_level.pl
> >
> > 3.
> > +$primary->safe_psql('postgres',
> > + qq[select pg_create_physical_replication_slot('test_phy_slot', false, false)]
> > +);
> > +
> > +# Check that creating a physical slot doesn't affect effective_wal_level.
> > +test_wal_level($primary, "replica|replica",
> > + "effective_wal_level doesn't change with a new physical slot");
> > +$primary->safe_psql('postgres',
> > + qq[select pg_drop_replication_slot('test_phy_slot')]);
> > +
> >
> > That pg_create_physical_replication_slot() should be beneath the
> > comment that says what the test is doing.
>
> Fixed.
>
> >
> > ~~~
> >
> > 4.
> > +# Create logical slots on the both nodes.
> >
> > typo /the both/both/
> >
> > ~~~
> >
> > 5.
> > +# effective_wal_level should be 'logical' on the both nodes.
> >
> > typo /the both/both/
> >
> > ~~~
> >
> > 6.
> > +# Verify that the effective_wal_level remains 'logical' on the both nodes
> >
> > typo /the both/both/
> >
>
> Actually, you previously made the opposite comment[1] but I've now
> confirmed that 'the' is unnecessary.
>

I've rebased the patch and addressed the review comments.

Regards,

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