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
