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

Hi Sawada-San.

Some review comments for v30-0001.

======
doc/src/sgml/system-views.sgml

1.
<para>
<literal>wal_level_insufficient</literal> means that the
- primary doesn't have a <xref linkend="guc-wal-level"/> sufficient to
- perform logical decoding. It is set only for logical slots.
+ primary doesn't have a <xref linkend="guc-effective-wal-level"/>
+ to perform logical decoding. It is set only for logical slots.
</para>

typo /doesn't have a/doesn't have an/

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

xlog_redo:

2.
+ /* Update the status on shared memory */
+ memcpy(&logical_decoding, XLogRecGetData(record), sizeof(bool));
+
+ /* The record must always change the status actually */
+ Assert(IsLogicalDecodingEnabled() != logical_decoding);
+
+ UpdateLogicalDecodingStatus(logical_decoding, true);

I felt that the comment "Update the status" is really referring to the
UpdateLogicalDecodingStatus() call, so it might be better to group
all those lines under a single comment.

SUGGESTION
/*
* Update the status (which must be a toggled value) on shared memory.
*/
memcpy(&logical_decoding, XLogRecGetData(record), sizeof(bool));
Assert(IsLogicalDecodingEnabled() != logical_decoding);
UpdateLogicalDecodingStatus(logical_decoding, true);

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

3.
case XLOG_PARAMETER_CHANGE:
+
+ /*
+ * a XLOG_PARAMETER_CHANGE record might change wal_level to
+ * 'replica' or 'minimal' but we don't check the logical decoding
+ * availability here because it's updated by a a
+ * XLOG_LOGICAL_DECODING_STATUS_CHANGE record.
+ */
+ break;

Typos:

/'minimal' but/'minimal', but/

/a XLOG_PARAMETER_CHANGE/A XLOG_PARAMETER_CHANGE/

/by a a/by a/

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

CheckLogicalDecodingRequirements:

4.
+ /* CheckSlotRequirements() has already checked if wal_level >= 'replica' */

This comment smells like it should be replaced with an Assert that
says the same thing.

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

5.
+ * In the future, we could extend support to include automatic transitions

Sawada-San reply to previous "XXX" comment:
Hmm, I've seen we use an "XXX" marker like "TODO" or "FIXME", but what
the comment explains is not actually a todo but possible future
enhancements.

~

You can search the source code for regex “XXX.*future” to find
multiple examples just like this.

~~~

6.
+/*
+ * A transaction local cache of XLogLogicalInfo:
+ * -1: not cached yet, need to check XLogLogicalInfo.
+ * 0: cached, XLogLogicalInfo is disabled.
+ * 1: cached, XLogLogicalInfo is enabled.
+ * The cache is set when checking XLogLogicalInfoActive() for the first time
+ * in the transaction and kept until the transaction end. Once the flag value
+ * is cached, this cache is used simply by casting to an boolean.
+ */
+int XLogLogicalInfoXactCache = -1;

6a.
Typo: /to an boolean./to a boolean./

~

6b.
Would it be better to have a self-documenting enum {UNKNOWN, ENABLED,
DISABLED} instead of some magic int values that need lots of comments
explaining how to use them?

~~~

LogicalDecodingCtlShmemInit:

7.
+void
+LogicalDecodingCtlShmemInit(void)
+{
+ bool found;
+
+ LogicalDecodingCtl = ShmemInitStruct("Logical decoding control",
+ LogicalDecodingCtlShmemSize(),
+ &found);
+
+ if (!found)
+ MemSet(LogicalDecodingCtl, 0, LogicalDecodingCtlShmemSize());
+}

Maybe assign LogicalDecodingCtlShmemSize() to some variable; no need
to invoke it 2x.

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

ReplicationSlotCleanup:

8.
+
+ if (SlotIsLogical(s) && s->data.invalidated == RS_INVAL_NONE)
+ found_valid_logicalslot = true;
+

I thought this might be optimised, so it does not bother to reassign
the same value.

SUGGESTION1:
if (!found_valid_logicalslot && SlotIsLogical(s) &&
s->data.invalidated == RS_INVAL_NONE)
found_valid_logicalslot = true;

SUGGESTION2:
found_valid_logicalslot |= (SlotIsLogical(s) && s->data.invalidated ==
RS_INVAL_NONE);

~~~

ReplicationSlotsDropDBSlots

9.
int i;
+ bool found_valid_logicalslot;
+ bool dropped = false;

This declares the same vars as that other function
ReplicationSlotCleanup(), so for consistency, you might as well
declare them in the same order...

~~~

10.
+ /*
+ * Count slots on other databases too so we can disable logical
+ * decoding only if no slots in the cluster.
+ */
+ SpinLockAcquire(&s->mutex);
+ found_valid_logicalslot |= (s->data.invalidated == RS_INVAL_NONE);
+ SpinLockRelease(&s->mutex);

The comment is old and needs updating because nothing is being
"counted" anymore.

~~~

InvalidatePossiblyObsoleteSlot:

11.
/* Let caller know */
- *invalidated = true;
+ invalidated = true;

This was a review v29 (#12) review comment that seems to have been
missed. That comment is stale (should be removed) because
'invalidated' is no longer returned by reference like before.

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