On Thu, Feb 6, 2025 at 7:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> 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.
>
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
