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-06 02:07:16 |
Message-ID: | OSCPR01MB149666529025043F58BEE24DAF5F62@OSCPR01MB14966.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Shubham,
Thanks for working on it. I have some comments for the patch.
01. fetch_all_databases
```
+/* Placeholder function to fetch all databases from the publisher */
+static void
+fetch_all_databases(struct CreateSubscriberOptions *opt)
```
Please update the comment atop function.
02. fetch_all_databases
```
+ /* Establish a connection to the PostgreSQL server */
+ conn = PQconnectdb(opt->pub_conninfo_str);
+ /* Check for connection errors */
+ if (PQstatus(conn) != CONNECTION_OK)
+ {
+ pg_log_error("connection to the PostgreSQL server failed: %s", PQerrorMessage(conn));
+ PQfinish(conn);
+ exit(1);
+ }
```
You can use connect_database() instead of directly calling PQconnectdb().
03. fetch_all_databases
```
+ const char *query = "SELECT datname FROM pg_database WHERE datistemplate = false";
```
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.
04. fetch_all_databases
```
+ /* Check if any databases were added */
+ if (opt->database_names.head == NULL)
+ {
+ pg_log_error("no database names could be fetched or specified");
+ pg_log_error_hint("Ensure that databases exist on the publisher or specify a database using --database option.");
+ PQclear(res);
+ PQfinish(conn);
+ exit(1);
+ }
```
It is enough to check num_rows > 0.
05. fetch_all_databases
```
+ PQfinish(conn);
```
Just in case: we can use disconnect_database().
06. validate_databases
```
+ /* Check for conflicting options */
+ if (opt->all_databases && opt->database_names.head != NULL)
+ {
+ pg_log_error("cannot specify --dbname when using --all-databases");
+ exit(1);
+ }
+
+ /*
+ * Ensure publication and slot names are not manually specified with
+ * --all-databases
+ */
+ if (opt->all_databases &&
+ (opt->pub_names.head != NULL || opt->replslot_names.head != NULL))
+ {
+ pg_log_error("cannot specify --publication or --replication-slot when using --all-databases");
+ exit(1);
+ }
```
I think validations should be done in main() like other paramters.
07. validate_databases
```
+ if (opt->all_databases)
+ {
```
This function is called when all_databases is true, so this is not needed.
08. validate_databases
```
+ cell = opt->database_names.head;
+
+ while (cell != NULL)
+ {
+ char slot_name[NAMEDATALEN];
+
+ snprintf(slot_name, sizeof(slot_name), "%s_slot", cell->val);
+ simple_string_list_append(&opt->replslot_names, slot_name);
+
+ snprintf(slot_name, sizeof(slot_name), "%s_pub", cell->val);
+ simple_string_list_append(&opt->pub_names, slot_name);
+
+ cell = cell->next;
+ }
```
I'm not sure the part. It seems to me that you tried to set slotname to ${dbname}_slot
and pubname to ${dbname}_pub, but this the current naming rule. Since we can
generate names in setup_publisher(), I think we do not have to do something here.
09.
```
@@ -2117,6 +2233,8 @@ main(int argc, char **argv)
}
}
+
+
```
Unnesesarry blank.
10. 040_pg_createsubscriber.pl
```
+# Fetch the count of non-template databases on the publisher before
+# running pg_createsubscriber without '--all-databases' option
+my $db_count_before =
+ $node_a->safe_psql('postgres',
+ "SELECT count(*) FROM pg_database WHERE datistemplate = false;");
+is($db_count_before, '1', 'database count without --all-databases option');
```
This test is not actually realted with our codes, no need to do.
11. 040_pg_createsubscriber.pl
```
+# Ensure there are some user databases on the publisher
+$node_a->safe_psql('postgres', 'CREATE DATABASE db3');
+$node_a->safe_psql('postgres', 'CREATE DATABASE db4');
+
+$node_b->stop;
```
You must wait until all changes have been replicated to the standby here.
12. 040_pg_createsubscriber.pl
Can we do the similar tests without adding new instances? E.g., runs with
dry-run mode and confirms all databases become target.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2025-02-06 02:28:52 | Re: Show WAL write and fsync stats in pg_stat_io |
Previous Message | Tom Lane | 2025-02-06 02:06:24 | Re: Show WAL write and fsync stats in pg_stat_io |