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

On Mon, Feb 10, 2025 at 6:58 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> > Fixed the given comments. The attached patch at [1] contains the
> > suggested changes.
>
> Thanks for updates. I registered the thread to the commitfest entry [1].
> Few comments.
>
> 01. fetch_source_databases
>
> ```
> + const char *query = "SELECT datname FROM pg_database WHERE datistemplate = false";
> ```
>
> You told given comments were fixed, but you ignored this. Please address it
> or clarify the reason why you didn't. My comment was:
>
> > We should verify the attribute datallowconn, which indicates we can connect to the
> > database. If it is false, no one would be able to connect to the db and define anything.
>

Fixed.

> 02. fetch_source_databases
> ```
> + /* Check for connection errors */
> + if (PQstatus(conn) != CONNECTION_OK)
> + {
> + pg_log_error("connection to the source server failed: %s", PQerrorMessage(conn));
> + disconnect_database(conn, false);
> + exit(1);
> + }
> ```
>
> connect_database() does the error handling, no need to do manually.
>

Fixed.

> 03. fetch_source_databases
> ```
> + pg_log_error("failed to execute query on the source server: %s", PQerrorMessage(conn));
> ```
>
> I feel the error message is too general. Also, PQerrorMessage() is not suitable for getting error messages
> generated by the query. How about:
> pg_log_error("could not obtain a list of databases: %s", PQresultErrorMessage());
>

Fixed.

> 04. fetch_source_databases
> ```
> + /* Error if no databases were found on the source server */
> + if (num_rows == 0)
> + {
> + pg_log_error("no databases found on the source server");
> + pg_log_error_hint("Ensure that there are user-created databases on the source server.");
> + PQclear(res);
> + disconnect_database(conn, false);
> + exit(1);
> + }
> ```
>
> Can you clarify when this happens?
>

This can happen when the postgres database has been dropped by
connecting to a template database.

> 05. main
>
> ```
> case 'd':
> + if (opt.all_databases)
> + {
> + pg_log_error("--database cannot be used with --all-databases");
> + exit(1);
> +
> ```
>
> I think this erroring is not enough. getopt_long() receives options with the specified ordering, thus
> -d can be accepted if the -a is specified latter.
> (Same thing can be said for 'P', '--replslot' and '--subscription'.)
>
> pg_createsubscriber ... -a -d postgres -> can be rejected,
> pg_createsubscriber ... -d postgres -a -> cannot be rejected.
>

Fixed.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.