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

Hi Sawada-San.

Here are my review comments for v23-0001. Most of them are cosmetic
comment suggestions.

======
src/backend/access/transam/xlog.c

show_effective_wal_level:

1.
+ /*
+ * During the recovery, instead of the local wal_level value, the
+ * primary's configuration is used for effective_wal_level. We need to
+ * check the shared status instead of local XLogLogicalInfo as we don't
+ * update it synchronously during the recovery.
+ */

SUGGESTION (reworded)
During recovery, effective_wal_level reflects the primary's
configuration rather than the local wal_level value. Check the shared
status instead of the local XLogLogicalInfo because XLogLogicalInfo is
not updated synchronously during recovery.

======
src/backend/replication/logical/logicalctl.c

UpdateLogicalDecodingStatus:

2.
+ /* It must be called before allowing the status change globally */
+ Assert(!LogicalDecodingCtl->status_change_allowed);

Reword the comment. Don't say "It".

~~~

start_logical_decoding_status_change:

3.
+/*
+ * This function does several kinds of preparation works required to start
+ * the process of logical decoding status change. If the status change is
+ * required, it sets LogicalDecodingCtl->status_change_inprogress, and
+ * returns true. Otherwise, if it's not required or not allowed (e.g., logical
+ * slots exist or during recovery) it returns false.
+ */

That first sentence "kinds of preparation works" sounds strange.

SUGGESTION:
Performs preparation work required before changing the logical decoding status.

~~~
RequestDisableLogicalDecoding:

4.
+/*
+ * Initiate a request for disabling logical decoding.
+ *
+ * This function expects to be called after dropping or invalidating the
+ * possibly-last logical replication slot as it doesn't check the
logical slot presence.
+ */
+void
+RequestDisableLogicalDecoding(void)

SUGGESTION (rearranged sentence)
This function does not verify whether logical slots exist. It should
be called after dropping or invalidating what might be the last
logical replication slot.

======
src/backend/replication/slot.c

ReplicationSlotRelease:

5.
+ /*
+ * Request to disable logical decoding even though this slot might
+ * have not been the last logical slot to drop. The checkpointer will
+ * check if logical decoding should be disabled afterward anyway.
+ */
+ if (is_logical)
+ RequestDisableLogicalDecoding();

There are some typos "to drop" and "afterward", but maybe it's better
just reword like below:

SUGGESTION:
Request logical decoding to be disabled, even though this slot may not
have been the last logical slot. The checkpointer will verify whether
logical decoding should actually be disabled.

~~~

CheckLogicalSlotExists:

6.
+bool
+CheckLogicalSlotExists(void)
+{
+ bool found = false;
+
+ if (max_replication_slots <= 0)
+ return false;
+
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+ for (int i = 0; i < max_replication_slots; i++)
+ {
+ ReplicationSlot *s;
+ bool invalidated;
+
+ s = &ReplicationSlotCtl->replication_slots[i];
+
+ /* cannot change while ReplicationSlotCtlLock is held */
+ if (!s->in_use)
+ continue;
+
+ if (SlotIsPhysical(s))
+ continue;
+
+ SpinLockAcquire(&s->mutex);
+ invalidated = s->data.invalidated != RS_INVAL_NONE;
+ SpinLockRelease(&s->mutex);
+
+ if (invalidated)
+ continue;
+
+ found = true;
+ break;
+ }
+ LWLockRelease(ReplicationSlotControlLock);
+
+ return found;
+}

Some inconsistent var usage. e.g. These functions have lots of
similarities but some say 'invalidated' and some say 'is_valid'.

~~~

InvalidatePossiblyObsoleteSlot

7.
Oid dboid, TransactionId snapshotConflictHorizon,
- bool *invalidated)
+ bool *released_lock_out)
...
- return released_lock;
+ *released_lock_out = released_lock;
+ return invalidated;

Is that output parameter 'released_lock_out' really needed? IIUC you
could do away with this parameter entirely, and just check
LWLockHeldByMe(ReplicationSlotControlLock) back where this function
(InvalidatePossiblyObsoleteSlot) was called?

~~~

8.
+ /*
+ * Request the checkpointer process to disable logical decoding if no
+ * valid logical slots exist. While the checkpointer can call this
+ * function during a checkpoint, it initiates the request but not do the
+ * actual deactivation so that it can complete the checkpointing first.
+ */

typo: "but not do the"

SUGGESTED (reworded)
Request the checkpointer to disable logical decoding if no valid
logical slots remain. If called by the checkpointer during a
checkpoint, only the request is initiated; actual deactivation is
deferred until after the checkpoint completes.

======
Kind Regards,
Peter Smith.
Fujitsu Australia