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
