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

Dear Shubham,

Thanks for updating the patch!

> I agree that ensuring the correct order of objects (like publications)
> matching with databases is crucial, especially when explicitly
> specifying databases using -d switches.
> To address this, I have created a 0002 patch that aligns object
> creation order with the corresponding databases.

ISTM 0002 does completely different stuff. It handles the case when dbname is not
specified in -P. Commit message is also wrong.

Anyway, comments for 0001/0002:

01. connect_database
```
@@ -536,8 +541,9 @@ connect_database(const char *conninfo, bool exit_on_error)
        conn = PQconnectdb(conninfo);
        if (PQstatus(conn) != CONNECTION_OK)
        {
-               pg_log_error("connection to database failed: %s",
-                                        PQerrorMessage(conn));
+               if (exit_on_error)
+                       pg_log_error("connection to database failed: %s",
+                                                PQerrorMessage(conn));
                PQfinish(conn);
```

Any error messages should not be suppressed. I imagine you've added this because
this command may try to connect to the postgres/template1, but this change affects
all other parts. I feel this change is not needed.

02. main()
```
+       if (opt.all_dbs)
+       {
+               bool            dbnamespecified = (dbname_conninfo != NULL);
+
+               get_publisher_databases(&opt, dbnamespecified);
+       }
        if (opt.database_names.head == NULL)
```

Need a blank between them. Ashutosh pointed out [1] because "else-if" was used in v21.
Now it becomes "if" again, the change must be reverted.

03. main()
```
-                * If --database option is not provided, try to obtain the dbname from
-                * the publisher conninfo. If dbname parameter is not available, error
-                * out.
+                * If neither --database nor --all option is provided, try to obtain
+                * the dbname from the publisher conninfo. If dbname parameter is not
+                * available, error out.
```

This comment must be updated because we can reach here even when -a is specified.
Personally, since the pg_log_info() describes the situation, it is enough;

Try to obtain the dbname from the publisher conninfo. If dbname parameter is not
available, error out.

04.
```
+# run pg_createsubscriber with '--all' option
+my ($stdout, $stderr) = run_command(
+       [
+               'pg_createsubscriber',
+               '--verbose',
+               '--dry-run',
+               '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+               '--pgdata' => $node_s->data_dir,
+               '--publisher-server' => $node_p->connstr($db1),
+               '--socketdir' => $node_s->host,
+               '--subscriber-port' => $node_s->port,
+               '--all',
+       ],
+       'run pg_createsubscriber with --all');
```

We should test the case when -P does not contain dbname. IIUC, it is enough to use
`node_p->connstr` instead of `node_p->connstr($db1)`.

[1]: https://www.postgresql.org/message-id/CAExHW5uvp6LWfgcysohDaOaNhqAbmuc%3D9BwWke%3D6KPRZ%2BwVOkA%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED