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

On Mon, Dec 15, 2025 at 10:32 PM Peter Smith <smithpb2250@gmail.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.

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoARX01kBsech_e_wETTQ9ZL%2BN-UC0OhTPvmYH9Zc1kdzA%40mail.gmail.com

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