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

Few comments:

1)
+ /*
+ * Logical decoding is normally disabled after dropping the last logical
+ * slot, but if it happens during process exit time for example when
+ * releasing a temporary logical slot on an error, the process sets this
+ * flag to true, delegating the checkpointer to disable logical decoding
+ * asynchronously.
+ */
+ bool pending_disable;

I do not see it happening while releasing a temporary logical slot on
an error (without process-exit). Also it happens on clean process-exit
(without hitting any ERROR). We should make the comment more clear.


2)
+ /*
+ * This flag is set to true by the startup process during recovery, to
+ * delay any logical decoding status change attempts until the recovery
+ * actually completes. The flag is set to true only during the recovery by
+ * the startup process. See comments in
+ * start_logical_decoding_status_change() for details.
+ */
+ bool delay_status_change;

The second line in the comment looks repetitive.

3)
+ if (!found)
+ {
+ LogicalDecodingCtl->xlog_logical_info = false;
+ LogicalDecodingCtl->logical_decoding_enabled = false;
+ LogicalDecodingCtl->status_change_inprogress = false;
+ LogicalDecodingCtl->pending_disable = false;
+ LogicalDecodingCtl->delay_status_change = false;
+ ConditionVariableInit(&LogicalDecodingCtl->cv);
+ }

Shall we do MemSet to 0 and then 'ConditionVariableInit' instead of
initializing all the fields to false?

4)
+ * Otherwise, if it's not required or not allowed (e.g., during recovery
+ * or wal_level = 'logical'), it returns false.
+  */
+static bool
+start_logical_decoding_status_change(bool new_status)
+{
+ Assert(!RecoveryInProgress());

We moved the 'recovery' and 'wal_level' checks outside but I think we
missed updating the comments here.

5)
+ /*
+ * When attempting to disable logical decoding, if there is at least one
+ * logical slots we cannot disable it.
+ */

little correction:
/*
 * When attempting to disable logical decoding, if there is at least one
 * logical slot, we cannot disable it.
 */

6)
+ * and slot creation. To ensure enabling logical decoding the caller

comma missing:
* and slot creation. To ensure enabling logical decoding, the caller

7)
+ if (RecoveryInProgress())
+ {
+ /*
+ * Check if we need to wait for the recovery completion. See the
+ * comments in check_wait_for_recovery_completion() for the reason why
+ * we check it here.
+ */
+ if (!check_wait_for_recovery_completion())
+ return;
+
+ wait_for_recovery_completion();
+ }

It would be helpful to also add that logical decoding changes are not
supported on a standby. Therefore, this function will be a no-op in
that scenario (provided there is no wait needed for recovery
completion).

I think this particular comment somehow got lost in all these iterations.

8)
+void
+DisableLogicalDecodingIfNecessary(bool complete_pending)

'complete_pending' looks a little odd to me. Shall we have 'finish_disable'?

thanks
Shveta