From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Shubham Khanna' <khannashubham1197(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
Subject: | RE: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. |
Date: | 2025-02-10 01:28:41 |
Message-ID: | OSCPR01MB14966DFD3A31B575781A595DEF5F22@OSCPR01MB14966.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
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.
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());
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?
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.
To avoid the issue, you must 1) add if-statements in 'a' case or 2) do validation outside of the
switch-case. I prefer 2.
[1]: https://commitfest.postgresql.org/52/5566/
Best regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2025-02-10 01:32:47 | Re: should we have a fast-path planning for OLTP starjoins? |
Previous Message | Michael Paquier | 2025-02-10 01:25:05 | Re: [PATCH] Optionally record Plan IDs to track plan changes for a query |