RE: speed up a logical replica setup

Dear Vignesh,

> 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');

Just to confirm - your suggestion is to add `pg_ctl status`, right? Added.

> 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');
> +

Hmm, in case of --dry-run, the server would be never shut down.
See below part.

```
if (!dry_run)
stop_standby_server(pg_ctl_path, opt.subscriber_dir);
```

> 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');

Fixed.

> 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]);

Fixed.

> 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'
> +);

Fixed, but "has" should be "have".

> 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

Hmm, but adding these checks may increase the test time. we should
measure the time and then decide.

> 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;

Fixed.

> 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;
>

Fixed. Also, recent commit [1] allows to run the initdb forcibly. So followed.

New patch can be available in [2].

[1]: https://github.com/postgres/postgres/commit/ff9e1e764fcce9a34467d614611a34d4d2a91b50
[2]: https://www.postgresql.org/message-id/TYCPR01MB12077CD333376B53F9CAE7AC0F5562%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/