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

On Mon, Mar 24, 2025 at 12:29 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> > I have reviewed and merged the proposed changes into the patch. The
> > attached patches contain the suggested changes.
>
> Thanks for updating the patch! Few comments:
>
> 01.
> ```
> + /*
> + * Fetch all databases from the source (publisher) if --all is specified.
> + */
> + if (opt.all_dbs)
> + fetch_source_databases(&opt);
> +
> if (opt.database_names.head == NULL)
> ```
>
> I feel we can convert "if" -> "else if" for the opt.database_names.head case,
> because fetch_source_databases() ensures databases are listed.
>

Fixed.

> 02.
> ```
> +# Set up node U as standby linking to node
> ```
>
> To confirm: why can we use "U" here?
>

Since node_s and node_t are already used in this test, I have used the
next letter, node_u. Additionally, comments have been added to improve
clarity.

> 03.
> ```
> +$node_u->append_conf(
> + 'postgresql.conf', qq[
> +primary_conninfo = '$pconnstr dbname=postgres'
> +]);
> ```
> I think this part is not required. init_from_backup() with has_streaming sets the
> primary_conninfo.
>

Fixed.

> 04.
> ```
> +# Stop $node_s
> +$node_s->stop;
> ```
>
> The comment does not describe anything. I feel you can say "Stop node S because
> it won't be used anymore", or just remove it.
>

Fixed.

> 05.
> ```
> +# Drop one of the databases (e.g., $db2)
> +$node_p->safe_psql('postgres', "DROP DATABASE \"$db2\"");
> ```
>
> "e.g." means "for example", so it is not suitable to put here. Please explain why it must be removed.
>

Fixed.

> 06.
> ```
> +# Get subscription names
> +$result = $node_u->safe_psql(
> + 'postgres', qq(
> + SELECT subname FROM pg_subscription WHERE subname ~ '^pg_createsubscriber_'
> +));
> +my @subnames1 = split("\n", $result);
> +
> +# Wait subscriber to catch up
> +$node_u->wait_for_subscription_sync($node_p, $subnames1[0]);
> +$node_u->wait_for_subscription_sync($node_p, $subnames1[1]);
> ```
>
> wait_for_subscription_sync() is used to wait until the initial synchronization is
> done, so not suitable here. wait_for_catchup() is more appropriate.
>

Fixed.

> 07.
> Also, the similar part exists on the pre-existing part (wait_for_subscription_sync
> was used there, but I feel this may not be correct). How about unifing them like
> attached?
>

Fixed.

> 08.
> ```
> +# Verify logical replication works for all databases
> +# Insert rows on node P
> +$node_p->safe_psql('postgres', "INSERT INTO tbl1 VALUES('first row')");
> ```
>
> This is confusing because 'fourth row' is inserted to $db1 just after it. How
> about the 'row in database postgres' or something?
>

Fixed.

The attached patches contain the suggested changes.

Thanks and regards,
Shubham Khanna.