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

Hi Sawada-San.

Some minor review comments for v35-0001.

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

ProcessBarrierUpdateXLogLogicalInfo:

1.
+/*
+ * This routine is called when we are told to update XLogLogicalInfo
+ * by a ProcSignalBarrier.
+ */
+bool
+ProcessBarrierUpdateXLogLogicalInfo(void)
+{
+ if (GetTopTransactionIdIfAny() != InvalidTransactionId)
+ {
+ /* Delay updating XLogLogicalInfo until the transaction end */
+ XLogLogicalInfoUpdatePending = true;
+ }
+ else
+ update_xlog_logical_info();
+
+ return true;
+}
+

Strange to have a boolean function that unconditionally returns true.
Should the function comment explain the meaning of the (always true)
return value?

The return value is assigned to a variable which defaults true anyway,
so really this function might as well be void.

~~~

EnsureLogicalDecodingEnabled:

2.
+ if (RecoveryInProgress())
+ {
+ /*
+ * CheckLogicalDecodingRequirements() must have already error out if
+ * logical decoding is not enabled since we cannot enable the logical
+ * decoding status during recovery.
+ */
+ Assert(IsLogicalDecodingEnabled());
+ return;
+ }

typo? "already error out"

======
.../recovery/t/050_effective_wal_level.pl

3.
+$primary->safe_psql('postgres',
+ qq[select pg_create_physical_replication_slot('test_phy_slot', false, false)]
+);
+
+# Check that creating a physical slot doesn't affect effective_wal_level.
+test_wal_level($primary, "replica|replica",
+ "effective_wal_level doesn't change with a new physical slot");
+$primary->safe_psql('postgres',
+ qq[select pg_drop_replication_slot('test_phy_slot')]);
+

That pg_create_physical_replication_slot() should be beneath the
comment that says what the test is doing.

~~~

4.
+# Create logical slots on the both nodes.

typo /the both/both/

~~~

5.
+# effective_wal_level should be 'logical' on the both nodes.

typo /the both/both/

~~~

6.
+# Verify that the effective_wal_level remains 'logical' on the both nodes

typo /the both/both/

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