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

On Wed, Oct 29, 2025 at 5:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 28, 2025 at 5:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I have noticed a few minor points:
> >
>
> Some more comments:
> 1.
> -  if (RecoveryInProgress())
> -  {
> -    /*
> -     * This check may have race conditions, but whenever
> -     * XLOG_PARAMETER_CHANGE indicates that wal_level has changed, we
> -     * verify that there are no existing logical replication slots. And to
> -     * avoid races around creating a new slot,
> -     * CheckLogicalDecodingRequirements() is called once before creating
> -     * the slot, and once when logical decoding is initially starting up.
> -     */
> -    if (GetActiveWalLevelOnStandby() < WAL_LEVEL_LOGICAL)
> -      ereport(ERROR,
> -          (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> -           errmsg("logical decoding on standby requires \"wal_level\"
> >= \"logical\" on the primary")));
> -  }
> +  /* CheckSlotRequirements() has already checked if wal_level >= 'replica' */
> +
> +  /*
> +   * Check if logical decoding is available on standbys. Note that
> +   * LogicalDecodingStatusChangeAllowed() returning false implies the
> +   * recovery is in-progress. See the comments atop logicalctl.c for
> +   * details.
> +   */
> +  if (!IsLogicalDecodingEnabled() && !LogicalDecodingStatusChangeAllowed())
>
>
> In this change, code and comments without the patch are easy to
> follow. IIUC, this is primarily to allow slot creation during the
> promotion. How about something like:
> "Check if logical decoding is available on standbys. Typically, when
> running a standby, RecoveryInProgress() returning true implies that
> LogicalDecodingStatusChangeAllowed() is false. However, during
> promotion, there's a brief transitional phase where
> RecoveryInProgress() remains true even though
> LogicalDecodingStatusChangeAllowed() has already turned true.
>
> In this window, logical decoding enable/disable operations are
> permitted on standby, anticipating its transition to primary. The
> actual wait for recovery completion is handled within
> start_logical_decoding_status_change()."
>
> 2
> + * Standby servers inherit the logical decoding and logical WAL writing status
> + * from the primary server.
>
> It sounds a bit odd to say that standby inherits logical WAL writing
> status from primary, as there is no WAL writing happening on standby.
>
> 3.
> other processes might attempt to
> + * enable or disable logical decoding before recovery fully completes
>
> Instead of enable or disable, it would be better to say toggle logical …
>
> 4.
> +/*
> + * Check the shared memory state and return true if logical information WAL
> + * logging is enabled.
> + */
>
> In this "...logical information WAL logging is enabled." sounds a bit
> odd. How about something like: "Returns true if logical WAL logging is
> enabled based on the shared memory state."?
>
> 5.
> EnsureLogicalDecodingEnabled() and DisableLogicalDecodingIfNecessary()
> + * should be used instead if there could be concurrent processes doing writes
> + * or logical decoding.
> + */
> +void
> +UpdateLogicalDecodingStatus(bool new_status, bool need_lock)
> …
>
> Can we ensure what is written in the above comment by an Assert?

Agreed with the above points, and fixed.

>
> 6.
> +DisableLogicalDecodingIfNecessary(void)
> +{
> + bool pending_disable;
> +
> + if (wal_level != WAL_LEVEL_REPLICA)
> + return;
> +
> + /*
> + * Sanity check as we cannot disable logical decoding while holding a
> + * logical slot.
> + */
> + Assert(!MyReplicationSlot);
>
> Is this assertion relevant now, considering we disable
> logical_decoding only via checkpointer?

I think it makes sense to have such an assertion just in case where
other processes call this function in the future. It's just a sanity
check as the comment says to prevent potential problems.

>
> 7.
> - if (SlotIsLogical(s))
> - nlogicalslots++;
> + if (SlotIsLogical(s) && s->data.invalidated == RS_INVAL_NONE)
> + n_valid_logicalslots++;
>
> Is it okay to check the invalidated flag without the spinlock? If so,
> it is better to write a comment as to why it is safe.

Fixed it since it needs to acquire the spinlock.

>
> 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.

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.

Regards,

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