Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. - Mailing list pgsql-hackers

On Thu, Feb 6, 2025 at 7:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@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
--databaseoption."); 
> +               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.