Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.

On Wed, Mar 19, 2025 at 2:29 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 19 Mar 2025 at 11:44, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> >
> > I agree with you on this; switching to a single query with safe_psql()
> > will indeed simplify the test and make the verification logic cleaner.
> >
> > The attached patch contains the suggested change.
>
> Few comments:
> 1) I felt we are not doing anything specific to dry-run, so no need to
> have this test case:
> +# run pg_createsubscriber with '--all' and '--database' and verify the
> +# failure
> +command_fails_like(
> + [
> + 'pg_createsubscriber',
> + '--verbose',
> + '--dry-run',
> + '--pgdata' => $node_s->data_dir,
> + '--publisher-server' => $node_p->connstr($db1),
> + '--socketdir' => $node_s->host,
> + '--subscriber-port' => $node_s->port,
> + '--all',
> + '--database' => $db1,
> + ],
> + qr/--database cannot be used with --all/,
> + 'fail if --database is used with --all');
> +
>

Fixed.

> 2) Similarly this too:
> +# run pg_createsubscriber with '--all' option
> +command_ok(
> + [
> + 'pg_createsubscriber',
> + '--verbose',
> + '--dry-run',
> + '--pgdata' => $node_s->data_dir,
> + '--publisher-server' => $node_p->connstr($db1),
> + '--socketdir' => $node_s->host,
> + '--subscriber-port' => $node_s->port,
> + '--all',
> + ],
> + 'run pg_createsubscriber with --all');
> +
>

Fixed.

> 3) We have run other pg_createsubscriber success tests with
> recovery-timeout specified, so I suggest modifying this too similarly:
> +# run pg_createsubscriber with '--all' option without '--dry-run'
> +command_ok(
> + [
> + 'pg_createsubscriber',
> + '--verbose',
> + '--pgdata' => $node_u->data_dir,
> + '--publisher-server' => $node_p->connstr($db1),
> + '--socketdir' => $node_u->host,
> + '--subscriber-port' => $node_u->port,
> + '--all',
> + ],
> + 'run pg_createsubscriber with --all');
> +
> +$node_u->start;
>

Fixed.

> 4) You can try to see how much extra time it takes to run the tests,
> if takes more time, then you can think of the following changes a) few
> of the option validation test cases can be removed, we can have just
> one combination of all with either of
> publication/subscription/replication slot b) for the last test added,
> you can drop one of the dbs and verify the subscriptions are created
> in 2 dbs as the code flow is the same for all databases.
>

I have created two patches, v16-0001 and v16-0002, to address the
performance issue. I conducted performance testing, and here are the
results:
- The difference in execution time between HEAD and the v15 patch was 53.2%.
- After removing the suggested test cases, the difference reduced to
36.43%, showing a significant improvement.

The optimizations included:
- Reducing redundant option validation test cases by retaining only
one combination of publication/subscription/replication slot.
- Dropping one of the databases in the last test case and verifying
that subscriptions are created correctly in the remaining two
databases, as the code flow remains consistent across databases.

The attached patches contain the suggested changes.

Thanks and regards,
Shubham Khanna.