On Mon, Feb 10, 2025 at 6:58 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.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.
v5-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
