On Thu, Nov 27, 2025 at 2:32 AM Masahiko Sawada <[email protected]> wrote:
>
>
> I've squashed all fixup patches and attached the updated patch.
>
Thanks for test results and the patch. Please find a few comments:
1)
If primary has effective_wal_level as logical and standby has zero
slots; then during promotion of standby, if I try to create a slot, it
is restricted as intended, but it gives misleading message:
postgres=# SELECT pg_create_logical_replication_slot('slot',
'pgoutput', false, false, false);
ERROR: logical decoding on standby requires "effective_wal_level" >=
"logical" on the primary
HINT: Set "wal_level" >= "logical" or create at least one logical
slot when "wal_level" = "replica".
One possibility is we extend the message as follows:
"Logical decoding on standby requires either effective_wal_level >=
logical on the primary or that recovery has finished." and remove
errhint?
2)
+ * completes. Here's how we handle concurrent toggling logical decoding:
toggling --> toggling of
3)
+ /*
+ * This is the authoritative value used by all processes to determine
+ * whether to write additional information required by logical decoding to
+ * WAL. Since this information could be checked frequently, each process
+ * caches this value in XLogLogicalInfo for better performance.
+ */
+ bool xlog_logical_info;
Shall we mention about cache XLogLogicalInfoXactCache as well here?
4)
EnsureLogicalDecodingEnabled():
+ if (wal_level == WAL_LEVEL_MINIMAL)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot enable logical decoding when \"wal_level\" = \"minimal\""));
Do we need this error here? CheckSlotRequirements checks this already.
Won't Assert suffice here?
5)
EnsureLogicalDecodingEnabled():
+ if (RecoveryInProgress())
+ {
+ Assert(IsLogicalDecodingEnabled());
+ return;
+ }
I understand that it will be no-op on standby. But it took me some
time to understand why we expect logical-decoding enabled here. I see
that CheckLogicalDecodingRequirements() won't allow the flow to reach
here if logical-decoding was not enabled in the first place. Shall we
add some comments here?
6)
DisableLogicalDecodingIfNecessary() has this:
+ if (wal_level != WAL_LEVEL_REPLICA)
+ return;
Also it has this:
+ /* Write the WAL to disable logical decoding on standbys too */
+ if (XLogStandbyInfoActive())
+ write_logical_decoding_status_update_record(false);
Do we actually need XLogStandbyInfoActive() here? Given that wal_level
is replica, and we’ve already confirmed that no slot exists while
pending_disable and xlog_logical_info are still true, isn’t that
enough to write a WAL record for the disable operation?
7)
+ /*
+ * With 'minimal' WAL level, there are not logical replication slots
+ * during recovery. Logical decoding is always disabled, so there is no
+ * need to synchronize XLogLogicalInfo..
+ */
not -> no
2 dots at the end.
=====
Reviewing more..
thanks
Shveta
