Re: speed up a logical replica setup

On Mon, 19 Feb 2024 at 11:15, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear hackers,
>
> > Since it may be useful, I will post top-up patch on Monday, if there are no
> > updating.
>
> And here are top-up patches. Feel free to check and include.
>
> v22-0001: Same as v21-0001.
> === rebased patches ===
> v22-0002: Update docs per recent changes. Same as v20-0002.
> v22-0003: Add check versions of the target. Extracted from v20-0003.
> v22-0004: Remove -S option. Mostly same as v20-0009, but commit massage was
>           slightly changed.
> === Newbie ===
> V22-0005: Addressed my comments which seems to be trivial[1].
>           Comments #1, 3, 4, 8, 10, 14, 17 were addressed here.
> v22-0006: Consider the scenario when commands are failed after the recovery.
>           drop_subscription() is removed and some messages are added per [2].
> V22-0007: Revise server_is_in_recovery() per [1]. Comments #5, 6, 7, were addressed here.
> V22-0008: Fix a strange report when physical_primary_slot is null. Per comment #9 [1].
> V22-0009: Prohibit reuse publications when it has already existed. Per comments #11 and 12 [1].
> V22-0010: Avoid to call PQclear()/PQfinish()/pg_free() if the process exits soon. Per comment #15 [1].
> V22-0011: Update testcode. Per comments #17- [1].

Few comments on the tests:
1) If the dry run was successful because of some issue then the server
will be stopped so we can check for "pg_ctl status" if the server is
running otherwise the connection will fail in this case. Another way
would be to check if it does not have "postmaster was stopped"
messages in the stdout.
+
+# Check if node S is still a standby
+is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
+       't', 'standby is in recovery');

2) Can we add verification of  "postmaster was stopped" messages in
the stdout for dry run without --databases testcase
+# pg_createsubscriber can run without --databases option
+command_ok(
+       [
+               'pg_createsubscriber', '--verbose',
+               '--dry-run', '--pgdata',
+               $node_s->data_dir, '--publisher-server',
+               $node_p->connstr('pg1'), '--subscriber-server',
+               $node_s->connstr('pg1')
+       ],
+       'run pg_createsubscriber without --databases');
+

3) This message "target server must be running" seems to be wrong,
should it be cannot specify cascading replicating standby as standby
node(this is for v22-0011 patch :
+               'pg_createsubscriber', '--verbose', '--pgdata',
$node_c->data_dir,
+               '--publisher-server', $node_s->connstr('postgres'),
+               '--port', $node_c->port, '--socketdir', $node_c->host,
+               '--database', 'postgres'
        ],
-       'primary server is in recovery');
+       1,
+       [qr//],
+       [qr/primary server cannot be in recovery/],
+       'target server must be running');

4) Should this be "Wait for subscriber to catch up"
+# Wait subscriber to catch up
+$node_s->wait_for_subscription_sync($node_p, $subnames[0]);
+$node_s->wait_for_subscription_sync($node_p, $subnames[1]);

5) Should this be 'Subscriptions has been created on all the specified
databases'
+);
+is($result, qq(2),
+       'Subscriptions has been created to all the specified databases'
+);

6) Add test to verify current_user is not a member of
ROLE_PG_CREATE_SUBSCRIPTION, has no create permissions, has no
permissions to execution replication origin advance functions

7) Add tests to verify insufficient max_logical_replication_workers,
max_replication_slots and max_worker_processes for the subscription
node

8) Add tests to verify invalid configuration of  wal_level,
max_replication_slots and max_wal_senders for the publisher node

9) We can use the same node name in comment and for the variable
+# Set up node P as primary
+$node_p = PostgreSQL::Test::Cluster->new('node_p');
+$node_p->init(allows_streaming => 'logical');
+$node_p->start;

10) Similarly we can use node_f instead of F in the comments.
+# Set up node F as about-to-fail node
+# Force it to initialize a new cluster instead of copying a
+# previously initdb'd cluster.
+{
+       local $ENV{'INITDB_TEMPLATE'} = undef;
+
+       $node_f = PostgreSQL::Test::Cluster->new('node_f');
+       $node_f->init(allows_streaming => 'logical');
+       $node_f->start;

Regards,
Vignesh