On Tue, Mar 25, 2025 at 3:22 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> > The attached patches contain the suggested changes.
>
> Thanks for updating the patch. I reviewed only 0001 because they would be committed separately.
> Few comments:
>
> 01.
> ```
> + For every non-template database on the source server, create one
> + subscription on the target server in the database with the same name.
> ```
>
> It is quite confusing for me; We do not have to describe users that this command
> checks databases of the source server. How about something like:
> "Create one subscription per all non-template databases on the target server."
>
Fixed.
> 02.
> ```
> +# run pg_createsubscriber with '--database' and '--all' without '--dry-run'
> +# and verify the failure
> +command_fails_like(
> + [
> + 'pg_createsubscriber',
> + '--verbose',
> + '--pgdata' => $node_s->data_dir,
> + '--publisher-server' => $node_p->connstr($db1),
> + '--socketdir' => $node_s->host,
> + '--subscriber-port' => $node_s->port,
> + '--database' => $db1,
> + '--all',
> + ],
> + qr/--database cannot be used with --all/,
> + 'fail if --database is used with --all');
> +
> +# run pg_createsubscriber with '--publication' 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,
> + '--all',
> + '--publication' => 'pub1',
> + ],
> + qr/--publication cannot be used with --all/,
> + 'fail if --publication is used with --all');
> ```
>
> You seemed to move most of validation checks to 0002. Can you tell me a reason
> why they are remained?
>
As per Amit's suggestions at [1], I moved most of the validation test
cases to the separate 0003 patch. However, these particular checks
were retained in the main patch because they ensure the fundamental
validations for incompatible options (--database and --publication
with --all) are covered early on.
> 03.
> ```
> +# Verify that the required logical replication objects are created. The
> +# expected count 3 refers to postgres, $db1 and $db2 databases.
> ```
>
> Hmm, but since we did a dry-run, any objects are not created, right?
>
Fixed.
The attached patches contain the suggested changes.
[1] - https://www.postgresql.org/message-id/CAA4eK1KUDEO0t6i16_CcEpg33sgcgEddHcdVC_q8j4tVUb5FWw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
