Hi Shubham,
Here are some review comments from my side
On Thu, Feb 13, 2025 at 8:58 AM Shubham Khanna
<khannashubham1...@gmail.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.
+ 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.
+
+# 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.
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.
--
Best Wishes,
Ashutosh Bapat
