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

On Tue, Aug 5, 2025 at 11:23 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Please find a few comments on v6:
>
> 1)
> +/*
> + * Initialize logical decoding status on shmem at server startup. This
> + * must be called ONCE during postmaster or standalone-backend startup,
> + * before initializing replication slots.
> + */
> +void
> +StartupLogicalDecodingStatus(bool last_status)
>
> The comment says that it needs to be called 'before initializing
> replication slots' but instead it is called after initializing
> replication slots (i.e. after StartupReplicationSlots).

Removed.

> Also, can you please help me understand the need of
> 'StartupLogicalDecodingStatus' when we are doing
> 'UpdateLogicalDecodingStatusEndOfRecovery' later in StartupXLOG. Why
> do we need to set last_status temporarily when the new status can be
> different which will be set in
> UpdateLogicalDecodingStatusEndOfRecovery

IIUC we need to initialize the logical decoding status with the status
we used to use when the server shutdown or when the basebackup was
taken. This status would be used during recovery and might be changed
by replaying the XLOG_LOGICAL_DECODING_STATUS_CHANGE record. At the
end of recovery, we update the status based on the server's wal_level
and the number of logical replication slots so the new status could be
different from the status used during the recovery.

>
>
> 2)
> CreatePublication() has this:
>
> + errmsg("logical decoding needs to be enabled to publish logical changes"),
> + errhint("Set \"wal_level\" to \"logical\" or create a logical
> replication slot with \"replica\" \"wal_level\" before creating
> subscriptions.")));
>
> While rest of the places has this:
>
> + errhint("Set \"wal_level\" >= \"logical\" or create at least one
> logical slot on the primary.")));
>
> Shall we make these errhint consistent?  Either all mention
> 'wal_level=replica' condition along with slot-creation part or none.

Fixed.

>
>
> 3)
> xlog_decode():
>
> + case XLOG_LOGICAL_DECODING_STATUS_CHANGE:
>   /*
>   * This can occur only on a standby, as a primary would
> - * not allow to restart after changing wal_level < logical
> + * not allow to restart after changing wal_level < replica
>   * if there is pre-existing logical slot.
>   */
>   Assert(RecoveryInProgress());
>   ereport(ERROR,
>   (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> - errmsg("logical decoding on standby requires \"wal_level\" >=
> \"logical\" on the primary")));
> + errmsg("logical decoding must be enabled on the primary")));
>
> Is the comment correct?
>
> a)
> XLOG_LOGICAL_DECODING_STATUS_CHANGE can be result of logical-slot drop
> on primary and not necessarily making wal_level < replica
>
> b)
> I see that even standby does not allow to restart when changing
> wal_level < replica as against what comment says. Have I understood
> the intent correctly?
>
> standby LOG:
> FATAL:  logical replication slot "failover_slot_st" exists, but
> "wal_level" < "replica"
> HINT:  Change "wal_level" to be "replica" or higher.

I think I don't get your point. The comment looks correct to me.

On a primary server, a logical slot can only decode WAL records that
were generated while that slot existed. A
XLOG_LOGICAL_DECODING_STATUS_CHANGE record with logical_decoding=false
is generated in two cases: after the last logical slot is dropped, or
when the server starts with no logical slot and wal_level<='replica'.
In either case, no logical slots can exist that would be able to
decode these WAL records. However, on standby servers, it is possible
to decode these XLOG_LOGICAL_DECODING_STATUS_CHANGE records with
logical_decoding=false, as standbys can decode WAL records
independently of the primary.

>
>
> 4)
> start_logical_decoding_status_change():
> + if (LogicalDecodingCtl->transition_in_progress)
> + {
> + LWLockRelease(LogicalDecodingControlLock);
>
> read_logical_decoding_status_transition() takes care of checking
> transition_in_progress, I think we missed to remove above from
> start_logical_decoding_status_change().

Fixed.

>
>
> 5)
> + /* Return if we don't need to change the status */
> + if (LogicalDecodingCtl->logical_decoding_enabled == new_status)
> + {
>
> Same with this code-logic in start_logical_decoding_status_change(),
> we shall remove it.

Fixed.

>
> 6)
> + * If we're in recovery and the startup process is still taking
> + * responsibility to update the status, we cannot change.
> + */
> + if (!delay_status_change)
> + return false;
> +
>
> This comment is confusing as when in recovery, we can not change state
> otherwise as well even if delay_status_change is false. IIUC, the
> scenario can arise only during promotion, if so, shall we say:
>
> "If we're in recovery and a state transition (e.g., promotion) is in
> progress, wait for the transition to complete and retry on the new
> primary. Otherwise, disallow the status change entirely, as a standby
> cannot modify the logical decoding status."

Fixed.

>
> 7)
> The name 'delay_status_change' does not indicate which status or the
> intent of delay. More name options are: defer_logical_status_change,
> wait_for_recovery_transition/completion,
> recovery_transition_in_progress

I think 'delay' is used in other similar examples in PostgreSQL code.
For instance, we have DELAY_CHKPT_START/COMPLETE/IN_COMMIT that are
set by transactions to delay the actual checkpoint process until these
transactions complete certain operations. In our case, the flag is set
by the startup process in order to delay the actual status change
process by other processes until the recovery completes. Which is a
very similar usage so I believe 'delay' is appropriate here.

Regarding the 'status', I guess it's relatively obvious in this
context that the status indicates the logical decoding status so I'm
not sure that readers would confuse this name.

>
> 8)
> DisableLogicalDecodingIfNecessary():
> +
> + /* Write the WAL to disable logical decoding on standbys too */
> + if (XLogStandbyInfoActive() && !recoveryInProgress)
> + {
>
> Do we need 'recoveryInProgress' check here?
> start_logical_decoding_status_change() has taken care of that.

Removed.

>
> 9)
> Comments atop DisableLogicalDecodingIfNecessary:
>
>  * This function expects to be called after dropping a possibly-last logical
>  * replication slot. Logical decoding can be disabled only when wal_level is set
>  * to 'replica' and there is no logical replication slot on the system.
>
> The comment is not completely true, shall we amend the comment to say
> something like:
>
> This function is called after a logical slot is dropped, but it only
> disables logical decoding on primary if it was the last remaining
> logical slot and wal_level < logical. Otherwise, it performs no
> action.

Thank you for the suggestion. I modified the comment based on the suggestion

>
> 10)
> When we try to create or drop a logical slot on standby, and if
> delay_status_change is false, shall we immediately exit? Currently it
> does a lot of checks including CheckLogicalSlotExists() which can be
> completely avoided. I think it is worth having a quick
> 'RecoveryInProgress() && !delay_status_change' check in the beginning.

Yeah, we can simplify the start_logical_decoding_status_change() logic more.

I've attached the updated patch.

Regards,

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