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

On Tue, Feb 18, 2025 at 7:32 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Few comments.
>
> ```
> + If a database is present on the source but missing on the target, an
> + error is raised.
> ```
>
> I'm not sure this description is accurate. I feel there is a case the command can
> succeed.
>
> Assuming that there are three databases (db1, db2, and db3) on the primary but db3
> is missing on the standby when DBA is running the command. In check_subscriber(),
> pg_createsubscriber connects to the db1 and it says OK. Then we wait until all
> changes are replicated, thus db3 is created on the target. Everything may go well afterward.
>
> Possible handling is as follows:
>
> 1) Ignore the corner case and retain,
> 2) Remove the description,
> 3) Modify the description, or
> 4) Modify codes to reject when the speficied database is missing on the target.
>
> I feel 2) may be enough, but I want to ask others as well.
>

I agree with you on this. Since there are cases where the command can
still succeed even if a database is initially missing on the target,
the description could be misleading. Based on this, I have removed
that part of the description.

>
> ```
> + If a database exists on the target but not on the source, no
> + subscription is created for it.
> ```
>
> I do not think this is needed. If the database exists on the target but not on the
> source, IIUC this means the DROP DATABASE was executed on the primary server but
> not replicated to the standby yet. In this case such databases would be dropped
> during the command because we ensure all changes are replicated before the promotion.
> Tell me if there are other situations...
>

I agree with your reasoning. Since any databases that exist on the
target but not on the source would be dropped as part of ensuring all
changes are replicated before promotion, explicitly mentioning this
scenario isn’t necessary. Based on this, I have removed that part of
the description.

> ```
> + disconnect_database(conn, false);
> + exit(1);
> ```
>
> This combination can be changed to disconnect_database(conn, true).
>

Fixed.

The attached patch at [1] contains the suggested changes.
[1] - https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.