On Thu, 7 Mar 2024 at 18:31, Shlok Kyal <[email protected]> wrote:
>
> Hi,
>
> > Thanks for the feedback. I'm attaching v26 that addresses most of your
> > comments
> > and some issues pointed by Vignesh [1].
>
> I have created a top-up patch v27-0004. It contains additional test
> cases for pg_createsubscriber.
>
> Currently, two testcases (in v27-0004 patch) are failing. These
> failures are related to running pg_createsubscriber on nodes in
> cascade physical replication and are already reported in [1] and [2].
> I think these cases should be fixed. Thoughts?
We can fix these issues, if we are not planning to fix any of them, we
can add documentation for the same.
> The idea of this patch is to keep track of testcases, so that any
> future patch does not break any scenario which has already been worked
> on. These testcases can be used to test in our development process,
> but which test should actually be committed, can be discussed later.
> Thought?
Few comments for v27-0004-Add-additional-testcases.patch:
1) We could use command_fails_like to verify the reason of the error:
+# set max_replication_slots
+$node_p->append_conf('postgresql.conf', 'max_replication_slots = 3');
+$node_p->restart;
+command_fails(
+ [
+ 'pg_createsubscriber', '--verbose',
+ '--dry-run', '--pgdata',
+ $node_s->data_dir, '--publisher-server',
+ $node_p->connstr('pg1'), '--socket-directory',
+ $node_s->host, '--subscriber-port',
+ $node_s->port, '--database',
+ 'pg1', '--database',
+ 'pg2',
+ ],
+ 'max_replication_slots are less in number in publisher');
2) Add a comment saying what is being verified
+# set max_replication_slots
+$node_p->append_conf('postgresql.conf', 'max_replication_slots = 3');
+$node_p->restart;
+command_fails(
+ [
+ 'pg_createsubscriber', '--verbose',
+ '--dry-run', '--pgdata',
+ $node_s->data_dir, '--publisher-server',
+ $node_p->connstr('pg1'), '--socket-directory',
+ $node_s->host, '--subscriber-port',
+ $node_s->port, '--database',
+ 'pg1', '--database',
+ 'pg2',
+ ],
+ 'max_replication_slots are less in number in publisher');
3) We could rename this file something like
pg_create_subscriber_failure_cases or something better:
src/bin/pg_basebackup/t/041_tests.pl | 285 +++++++++++++++++++++++++++
1 file changed, 285 insertions(+)
create mode 100644 src/bin/pg_basebackup/t/041_tests.pl
diff --git a/src/bin/pg_basebackup/t/041_tests.pl
b/src/bin/pg_basebackup/t/041_tests.pl
new file mode 100644
index 0000000000..2889d60d54
--- /dev/null
+++ b/src/bin/pg_basebackup/t/041_tests.pl
@@ -0,0 +1,285 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
4) This success case is not required as this would have already been
covered in 040_pg_createsubscriber.pl:
+$node_p->append_conf('postgresql.conf', 'max_replication_slots = 4');
+$node_p->restart;
+command_ok(
+ [
+ 'pg_createsubscriber', '--verbose',
+ '--dry-run', '--pgdata',
+ $node_s->data_dir, '--publisher-server',
+ $node_p->connstr('pg1'), '--socket-directory',
+ $node_s->host, '--subscriber-port',
+ $node_s->port, '--database',
+ 'pg1', '--database',
+ 'pg2',
+ ],
+ 'max_replication_slots are accurate on publisher');
5) We could use command_fails_like to verify the reason of the error:
$node_s->append_conf('postgresql.conf', 'max_replication_slots = 1');
$node_s->restart;
command_fails(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'max_replication_slots are less in number in subscriber');
6) Add a comment saying what is being verified
$node_s->append_conf('postgresql.conf', 'max_replication_slots = 1');
$node_s->restart;
command_fails(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'max_replication_slots are less in number in subscriber');
7) This success case is not required as this would have already been
covered in 040_pg_createsubscriber.pl:
$node_s->append_conf('postgresql.conf', 'max_replication_slots = 2');
$node_s->restart;
command_ok(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'max_replication_slots are less in number in subscriber');
8) We could use command_fails_like to verify the reason of the error:
# set wal_level on publisher
$node_p->append_conf('postgresql.conf', 'wal_level = \'replica\'');
$node_p->restart;
command_fails(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'wal_level must be logical');
9) Add a comment saying what is being verified
# set wal_level on publisher
$node_p->append_conf('postgresql.conf', 'wal_level = \'replica\'');
$node_p->restart;
command_fails(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'wal_level must be logical');
10) This success case is not required as this would have already been
covered in 040_pg_createsubscriber.pl:
$node_p->append_conf('postgresql.conf', 'wal_level = \'logical\'');
$node_p->restart;
command_ok(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'wal_level is logical');
11) We could use command_fails_like to verify the reason of the error:
# set max_wal_senders on publisher
$node_p->append_conf('postgresql.conf', 'max_wal_senders = 2');
$node_p->restart;
command_fails(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'max_wal_senders is not sufficient');
12) Add a comment saying what is being verified:
# set max_wal_senders on publisher
$node_p->append_conf('postgresql.conf', 'max_wal_senders = 2');
$node_p->restart;
command_fails(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'max_wal_senders is not sufficient');
13) This success case is not required as this would have already been
covered in 040_pg_createsubscriber.pl:
$node_p->append_conf('postgresql.conf', 'max_wal_senders = 3');
$node_p->restart;
command_ok(
[
'pg_createsubscriber', '--verbose',
'--dry-run', '--pgdata',
$node_s->data_dir, '--publisher-server',
$node_p->connstr('pg1'), '--socket-directory',
$node_s->host, '--subscriber-port',
$node_s->port, '--database',
'pg1', '--database',
'pg2',
],
'max_wal_senders is sufficient');
14) This sleep is not required
# max_worker_processes on subscriber
$node_p->append_conf('postgresql.conf', 'max_worker_processes = 2');
$node_p->restart;
sleep 1;
$node_s->append_conf('postgresql.conf', 'max_worker_processes = 2');
$node_s->restart;
15) The similar comments exist in other places also, I'm not repeating them.
Regards,
Vignesh
