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

On Fri, Feb 14, 2025 at 5:27 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Shubham,
> Here are some review comments from my side
>
>
> On Thu, Feb 13, 2025 at 8:58 AM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> > The attached patch contains the required changes.
> >
>
> clusterdb, vacuumdb use -a and -all for all databases. Do we want to
> use the same long option name here? Probably they don't have
> -databases in the long option name since db is already in their name.
> pg_createsubscriber doesn't have db in its name. But still it might be
> better to have just -all to be consistent with those utilities. Sorry
> if this has already been discussed.
>

To maintain consistency with clusterdb and vacuumdb, I have changed
the option name from '--all-databases' to '--all'.

> + printf(_(" -a, --all-databases create subscriptions on the target
> for all source databases\n"));
>
> Suggest "create subscriptions for all non-template source databases".
> "on the target" seems unnecessary, it's implicit.
>

Fixed.

> +
> +# run pg_createsubscriber with '--database' and '--all-databases' 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,
> + '--database' => $db1,
> + '--all-databases',
> + ],
> + qr/--all-databases cannot be used with --database/,
> + 'fail if --all-databases is used with --database');
>
> Need a test where --all-databases is specified before --database.
>

Added a test case for this.

> All the tests run in --dry-mode which won't test whether all the
> replication slots, subscriptions and publications are working well
> when --all-databases is specified. I think we need to test non-dry
> mode creation. I may go as far as suggesting to split the current test
> file into 3, 1. tests sanity of command line options 2. tests single
> subscription creation thoroughly along with multiple --database
> specifications 3. tests --all-databases health of the subscriptions
> and --all-databases specific scenarios like non-template databases
> aren't subscribed to.
>
> --

I have added a test case for non-dry-run mode to ensure that
replication slots, subscriptions, and publications work as expected
when '--all' is specified. Additionally, I have split the test file
into two parts:
1) Command-line sanity checks – Validates the correctness of option parsing.
2) '--all' functionality and behavior – Verifies the health of
subscriptions and ensures that --all specific scenarios, such as
non-template databases not being subscribed to, are properly handled.
This should improve test coverage while keeping the structure manageable.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.