Re: POC: enable logical decoding when wal_level = 'replica' without a server restart - Mailing list pgsql-hackers

Hi Sawada-Sa,

Some review comments for the patch v29-0001.

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

1.
+int effective_wal_level = WAL_LEVEL_REPLICA;

Should that be declared as WalLevel instead of int?

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

xlog_decode:

2.
+
+ /*
+ * Even if wal_level on the primary got decreased to 'replica', as
+ * long as there is at least one valid logical slot, the logical
+ * decoding remains enabled. So we don't check the logical
+ * decoding availability here but do so in
+ * XLOG_LOGICAL_DECODING_STATUS_CHANGE case. It covers the case
+ * where wal_level on the primary got decreased to 'minimal' too.
+ */
+ break;

This comment seems to be written more like a comparison with what the
comment used to say -- e.g. to me this reads like "We don't need to do
the check we used to do here anymore because...". Maybe it can be
worded slightly differently?

~~

3.
+ bool    *logical_decoding = (bool *) XLogRecGetData(buf->record);

- /*
- * If wal_level on the primary is reduced to less than
- * logical, we want to prevent existing logical slots from
- * being used.  Existing logical slots on the standby get
- * invalidated when this WAL record is replayed; and further,
- * slot creation fails when wal_level is not sufficient; but
- * all these operations are not synchronized, so a logical
- * slot may creep in while the wal_level is being reduced.
- * Hence this extra check.
- */
- if (xlrec->wal_level < WAL_LEVEL_LOGICAL)
+ if (!(*logical_decoding))

Would it be simpler just to declare 'logical_decoding' as bool and
deal with all the pointers during the assignment?

e.g.
BEFORE
bool *logical_decoding = (bool *) XLogRecGetData(buf->record);
if (!(*logical_decoding))

AFTER
bool logical_decoding = *((bool *) XLogRecGetData(buf->record));
if (!logical_decoding)

~~

4.
  }
+
  break;

Was the added blank line before 'break;' deliberate?

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

CheckLogicalDecodingRequirements:

5.
+ /* CheckSlotRequirements() has already checked if wal_level >= 'replica' */
+
+ /* Check if logical decoding is available on standby */
+ if (RecoveryInProgress() && !IsLogicalDecodingEnabled())

Should both those single-line comments be combined into one larger comment?

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

6.
+ * In the future, we could extend support to include automatic transitions
+ * of effective_wal_level between 'minimal' and 'logical' WAL levels. However,
+ * this enhancement would require additional coordination mechanisms and
+ * careful implementation of operations such as terminating walsenders and
+ * archiver processes while carefully considering the sequence of operations
+ * to ensure system stability during these transitions.

By convention, I think a comment that says "In the future" should also
have an "XXX" marker.

~~~

7.
+/*
+ * Writes XLOG_LOGICAL_DECODING_STATUS_CHANGE WAL record with the given status.
+ */
+static void
+write_logical_decoding_status_update_record(bool status)

/Writes/Writes an/

~~~

DisableLogicalDecodingIfNecessary:

8.
+ ereport(LOG,
+ errmsg("logical decoding is disabled because there is no valid
logical replication slot"));

SUGGESTION
logical decoding is disabled because there are no valid logical
replication slots

~~

UpdateLogicalDecodingStatusEndOfRecovery:

9.
+ /*
+ * With 'minimal' WAL level, there have not been logical slots during
+ * recovery. Logical decoding is always disabled, and no need to
+ * synchronize XLogLogicalInfo.
+ */

The wording of the comment didn't seem right.

SUGGESTION
With 'minimal' WAL level, there are no logical replication slots
during recovery. Logical decoding is always disabled, so there is no
need to synchronize XLogLogicalInfo.

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

ReplicationSlotCleanup:

10.
+
+ if (SlotIsLogical(s) && s->data.invalidated == RS_INVAL_NONE)
+ n_valid_logicalslots++;
+

Since you are not actually doing anything with this counter, other
than later checking if it is 0, the counter could be replaced just by
a boolean 'found_valid_logicalslot', so the code could be made
slightly more performant if needed.

~~~

ReplicationSlotsDropDBSlots:

11.

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

Same as the previous review comment #10. This counter could be
replaced by a boooelan if you want.

e.g.
found_valid_logicalslot |= (s->data.invalidated == RS_INVAL_NONE);

~~~

InvalidatePossiblyObsoleteSlot:

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

That comment is old from when 'invalidated' was a parameter by
reference. It doesn't seem appropriate now.

~~~

InvalidateObsoleteReplicationSlots:

13.
+ SpinLockAcquire(&s->mutex);
+ if (s->data.invalidated == RS_INVAL_NONE)
+ n_valid_logicalslots++;
+ SpinLockRelease(&s->mutex);
+

Yet another place (same as above review comments #10,#11) where the
counter could be replaced by a simple boolean.

found_valid_logicalslot |= (s->data.invalidated == RS_INVAL_NONE);

~~~

14.
+ /*
+ * Additionally, remember we have invalidated a logical slot too
+ * as we can request disabling logical decoding later.
+ */

Don't say "Additionally" and "too" in the same sentence.

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