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

Hi Sawada-San,

Some review comments for v22-0002:

======
doc/src/sgml/logicaldecoding.sgml

1.
      consistency. Conversely, when the last logical replication slot is dropped
-     from a system with <varname>wal_level</varname> set to
<literal>replica</literal>,
-     logical decoding is automatically disabled. Note that the deactivation of
-     logical decoding might take some time as it is performed asynchronously
-     by the checkpointer process.
+     from a system or invalidated with <varname>wal_level</varname> set to
+     <literal>replica</literal>, logical decoding is automatically disabled.
+     Note that the deactivation of logical decoding might take some time as it
+     is performed asynchronously by the checkpointer process.
     </para>

I felt that "Conversely..." sentence should be worded more like the
1st sentence of the paragraph.

CURRENT
Conversely, when the last logical replication slot is dropped from a
system or invalidated with <varname>wal_level</varname> set to
<literal>replica</literal>, logical decoding is automatically
disabled.

SUGGESTION
Conversely, if <varname>wal_level</varname> is
<literal>replica</literal> and  the last logical replication slot is
dropped or invalidated, logical decoding is automatically disabled.

(Aside: most of this suggestion maybe belongs for patch 0001 -- only
the "or invalidated" part is added for patch 0002)

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

ReplicationSlotsDropDBSlots:

2.
- nlogicalslots++;
+ if (s->data.invalidated == RS_INVAL_NONE)
+ n_valid_logicalslots++;

  /* not our database, skip */
  if (s->data.database != dboid)
  continue;

Should that counter increment be moved to be *below* the "not our
database, skip" code?

~~~

CheckLogicalSlotExists:

3.
 /*
- * Returns true if there is at least in-use logical replication slot.
+ * Returns true if there is at least in-use valid logical replication slot.
  */
 bool
 CheckLogicalSlotExists(void)

Typo? /is at least in-use valid/is at least one in-use valid/

~~~

4.
+ if (SlotIsLogical(s))
+ {
+ if (s->data.invalidated == RS_INVAL_NONE)
+ n_valid_logicalslots++;
+
+ /* Prevent invalidation of logical slots during binary upgrade */
+ if (IsBinaryUpgrade)
+ continue;
+ }

Is it better to do the binary check first, before the n_valid_logicalslots++?

======
src/test/recovery/t/049_effective_wal_level.pl

5.
-# Promote the standby4 and check if effective_wal_level remains 'logical' even
-# after the promotion since it has an invalidated logical slot.
+# Promote the standby4 and check if effective_wal_level is now 'logical' after
+# the promotion since there is no valid logical slot.
 $standby4->promote;
-test_wal_level($standby4, "replica|logical",
- "effective_wal_level remains 'logical' even after promotion as it
has one invalidated slot"
+test_wal_level($standby4, "replica|replica",
+ "effective_wal_level got decreased to 'replica' as there is no valid
logical slot"
 );

The test comment seems wrong; shouldn't it say "check if
effective_wal_level_got decreased to replica"?

~~~

6.
 # Drop the invalidated slot, decreasing effective_wal_level to 'replica'.
@@ -287,7 +326,7 @@ $standby4->safe_psql('postgres',
  qq[select pg_drop_replication_slot('standby4_slot')]);
 wait_for_logical_decoding_disabled($standby4);
 test_wal_level($standby4, "replica|replica",
- "effective_wal_level got decreased to 'replica' after dropping the
last invalidated slot"
+ "effective_wal_level doesn't change after dropping the last invalidated slot"
 );

The test comment seems wrong; effective_wal_level was already
'replica' before this test.

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