Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. - Mailing list pgsql-hackers

On Thu, Feb 20, 2025 at 1:23 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham, Here are some review comments for v9-0001.
>
> ======
> Commit message
>
> 1.
> You've changed the option to '--all' but the comment message still
> refers to '-all-databases'
>

Fixed.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 2.
> +      <para>
> +       Automatically fetch all non-template databases from the source server
> +       and create subscriptions for databases with the same names on the
> +       target server.
>
> I don't see what is "automatic" about this, nor do I know really what
> "fetch" of databases is trying to convey.
>
> Isn't it simpler to just say like below?
>
> SUGGESTION
> For all source server non-template databases create subscriptions for...
>

Fixed.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 3.
>   int recovery_timeout; /* stop recovery after this time */
> + bool all; /* --all option was specified */
>  };
>
>
> IMO 'all' is too generic for a field name; it has almost no meaning --
> all what? I think the original 'all_databases' name was better. Or
> 'all_dbs', but not just 'all'.
>
> ~~~

Fixed.

>
> 4.
> + if (opt.all)
> + {
> + if (num_dbs > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--database");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
> +
> + if (num_pubs > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--publication");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
> +
> + if (num_replslots > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--replication-slot");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
> +
> + if (num_subs > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--subscription");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
>
> 4a
> If bad combinations exist then IMO it is most likely that the --all
> came before the incompatible option. So the parameters should be the
> other way around so the result is "--<XXX> cannot be used with --all".
>
> ~

Fixed.

>
> 4b.
> You could eliminate all this code duplication by using a variable for
> the bad option,
>
> SUGGESTION
> char *bad_switch = NULL;
>
> if (num_dbs > 0)            bad_switch = "--database";
> else if (num_pubs > 0)      bad_switch = "--publication";
> else if (num_replslots > 0) bad_switch = "--replication_slot";
> else if (num_subs > 0)      bad_switch = "--subscription";
>
> if (bad_switch)
> {
>   pg_log_error("%s cannot be used with --all", bad_switch);
>   pg_log_error_hint("Try \"%s --help\" for more information.", progname);
>   exit(1);
> }
>
> ~
>

Fixed.

> 4c.
> There is a double-blank line after this code fragment.
>

Fixed.

> ======
> .../t/040_pg_createsubscriber.pl
>
> 5.
> +# run pg_createsubscriber with '--database' and '--all' 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',
> + ],
> + qr/--all cannot be used with --database/,
> + 'fail if --all is used with --database');
> +
>
> There is no difference anymore in the logic/error if the options are
> given in order "--all --database"  or if they are in order "--database
> --all". So you no longer need to have separate test cases for
> different ordering.
>
> ~~~

Fixed.

>
> 6.
> +# Verify that only user databases got subscriptions (not template databases)
> +my @user_dbs = ($db1, $db2);
> +foreach my $dbname (@user_dbs)
> +{
> + my $sub_exists =
> +   $node_s1->safe_psql($dbname, "SELECT count(*) FROM pg_subscription;");
> + is($sub_exists, '3', "Subscription created successfully for $dbname");
> +}
>
> AFAICT this is just checking a global subscription count in a loop so
> it will be 3, 3, 3. Why?
>
> I guess you intended to verify that relevant subscriptions were
> created for each of the target databases. But this code is not doing
> that.
>
> ~~~

Fixed.

>
> 7.
> +# Verify replication is working
> +$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
> +
> +$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
> +is( $result, qq(first row
> +replication test
> +second row), "replication successful in $db1");
> +
> +$node_s1->stop;
>  # Run pg_createsubscriber on node S.  --verbose is used twice
>  # to show more information.
>  command_ok(
> @@ -431,8 +590,9 @@ $result = $node_s->safe_psql($db1,
>  is($result, qq(0), 'failover slot was removed');
>
>  # Check result in database $db1
> -$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1');
> +$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1 ORDER BY 1');
>  is( $result, qq(first row
> +replication test
>
> I concur with Kuroda-san's comment. Maybe remove all this, because
> AFAICT existing code was already testing that replication is working
> ok for $db1 and $db2.
>
> ======

Fixed.

The attached patch at [1] contains the suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8RjLtgrA9odXtwhit1mUfqogNSF4qhkvDrPbxEoWba%2B4SOw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.