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

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

In response to

Responses

Browse pgsql-hackers by date

  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