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

Hi Sawada-San.

I reviewed the test code of patch v19; below are some minor comments.

======

1.
+# Initialize the primary server with wal_level = 'replica'.
+my $primary = PostgreSQL::Test::Cluster->new('primary');
+$primary->init(allows_streaming => 1);
+$primary->append_conf('postgresql.conf', "log_min_messages = debug1");
+$primary->start();

Would it be nicer to set wal_level  explicitly? Otherwise, maybe the
comment should say that 'replica' is the default.

~~~

2.
+# Check both initial wal_level and effective_wal_level values.
+test_wal_level($primary, "replica|replica",
+ "wal_level and effective_wal_level starts with the same value 'replica'");
+

/starts/start/

~~~

3.
+# Create a temporary logical slot and exits without releasing it explicitly.
+# This enables logical decoding but skips disabling it and delegates to the
+# checkpointer.
+$primary->safe_psql('postgres',
+ qq[select pg_create_logical_replication_slot('test_tmp_slot',
'test_decoding', true)]
+);
+
+# Wait for the checkpointer to disable logical decoding.
+wait_for_logical_decoding_deactivation($primary);
+

3a.
typo? /and exits/but exit/

~

3b.
I had expected to see the wal_level|effective_wal_level values getting
checked before/after that deactivation.

~~~

4.
+# Promote the standby1 node that doesn't have any logical slot. So
+# effective_wal_level should be decreased to 'replica' at promotion.
+$standby1->promote;
+test_wal_level($standby1, "replica|replica",
+ "effective_wal_level got decrased to 'replica' during promotion");
+$standby1->stop;
+

typo: /decrased/decreased/

~~~

5.
+# Initialize standby3 node and starts it with wal_level = 'logical'.
+my $standby3 = PostgreSQL::Test::Cluster->new('standby3');
+$standby3->init_from_backup($primary, 'my_backup', has_streaming => 1);
+$standby3->set_standby_mode();
+$standby3->append_conf('postgresql.conf', qq[wal_level = 'logical']);
+$standby3->start();
+$standby3->backup('my_backup3');
+

/starts/start/

~~~

6.
+# Drop the primary's last logical slot, decreasing effective_wal_level to
+# replica on all nodes.
+$primary->safe_psql('postgres',
+ qq[select pg_drop_replication_slot('test_slot')]);
+wait_for_logical_decoding_deactivation($primary);
+
+$primary->wait_for_replay_catchup($standby3);
+$standby3->wait_for_replay_catchup($cascade, $primary);
+
+test_wal_level($primary, "replica|replica",
+ "effective_wal_level got decreased to 'replica' on primary");
+test_wal_level($standby3, "logical|replica",
+ "effective_wal_level got decreased to 'replica' on standby");
+test_wal_level($cascade, "replica|replica",
+ "effective_wal_level got decreased to 'replica' on cascaded standby");
+

The "logical|replica" combination looks a bit weird; maybe it needs
more explanation in the comment.

~~~

7.
+# Initialize standby4 node and starts it with wal_level = 'logical'.
+my $standby4 = PostgreSQL::Test::Cluster->new('standby4');
+$standby4->init_from_backup($primary, 'my_backup', has_streaming => 1);
+$standby4->set_standby_mode();
+$standby4->start;
+

7a.
/starts/start/

~

7b.
I got confused here. Isn't the $primary still wal_level=replica? But
this comment says starting wal_level = 'logical'. Is it correct?

~~~

8.
+# Drop the logical slot from the primary, decreasing effective_wal_level to
+# 'replica' on the primary, which leads to invalidate the logical
slot on the standby
+# due to 'wal_level_insufficient'.
+$primary->safe_psql('postgres',
+ qq[select pg_drop_replication_slot('test_slot')]);
+wait_for_logical_decoding_deactivation($primary);
+test_wal_level($primary, "replica|replica",
+ "effective_wal_level got decreased to 'replica' on the primary to
invalidate standby's slots"
+);
+$standby4->poll_query_until(
+ 'postgres', qq[
+select invalidation_reason = 'wal_level_insufficient' from
pg_replication_slots where slot_name = 'standby4_slot'
+     ]);
+

8a.
/invalidate/invalidating/

~

8b.
Should this part also be showing the expected
wal_level|effective_wal_level on the $standby4?

~~~

9.
+# Check if we cannot use the invalidated slot on the standby.
+($result, $stdout, $stderr) = $standby4->psql('postgres',
+ qq[select pg_logical_slot_get_changes('standby4_slot', null, null)]);
+ok( $stderr =~
+   /ERROR:  can no longer access replication slot "standby4_slot"/,
+ "cannot use invalidated logical slot");
+

/Check if/Check that/

~~~

10.
+ # Initialize standby5 and starts it with wal_level = 'logical'.
+ my $standby5 = PostgreSQL::Test::Cluster->new('standby5');
+ $standby5->init_from_backup($primary, 'my_backup', has_streaming => 1);
+ $standby5->set_standby_mode();
+ $standby5->start;
+

/starts it/start it/

~~~

11.
+ # Start the logical decoding activation process upon creating the logical
+ # slot, but it will wait due to the injection point.
+ $psql_create_slot_1->query_until(
+ qr/create_slot_cancelled/,
+ q(\echo create_slot_cancelled
+select injection_points_set_local();
+select injection_points_attach('logical-decoding-activation', 'wait');
+select pg_create_logical_replication_slot('slot_cancelled', 'pgoutput');
+\q
+));
+

I found everywhere in the PG source uses the American spelling
"canceled" (one L), so this patch should too in all places.

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