From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Shubham Khanna' <khannashubham1197(at)gmail(dot)com> |
Cc: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, vignesh C <vignesh21(at)gmail(dot)com> |
Subject: | RE: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. |
Date: | 2025-03-24 06:59:06 |
Message-ID: | OSCPR01MB14966083F0E05523FDBEE4267F5A42@OSCPR01MB14966.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Shubham,
> I have reviewed and merged the proposed changes into the patch. The
> attached patches contain the suggested changes.
Thanks for updating the patch! Few comments:
01.
```
+ /*
+ * Fetch all databases from the source (publisher) if --all is specified.
+ */
+ if (opt.all_dbs)
+ fetch_source_databases(&opt);
+
if (opt.database_names.head == NULL)
```
I feel we can convert "if" -> "else if" for the opt.database_names.head case,
because fetch_source_databases() ensures databases are listed.
02.
```
+# Set up node U as standby linking to node
```
To confirm: why can we use "U" here?
03.
```
+$node_u->append_conf(
+ 'postgresql.conf', qq[
+primary_conninfo = '$pconnstr dbname=postgres'
+]);
```
I think this part is not required. init_from_backup() with has_streaming sets the
primary_conninfo.
04.
```
+# Stop $node_s
+$node_s->stop;
```
The comment does not describe anything. I feel you can say "Stop node S because
it won't be used anymore", or just remove it.
05.
```
+# Drop one of the databases (e.g., $db2)
+$node_p->safe_psql('postgres', "DROP DATABASE \"$db2\"");
```
"e.g." means "for example", so it is not suitable to put here. Please explain why it must be removed.
06.
```
+# Get subscription names
+$result = $node_u->safe_psql(
+ 'postgres', qq(
+ SELECT subname FROM pg_subscription WHERE subname ~ '^pg_createsubscriber_'
+));
+my @subnames1 = split("\n", $result);
+
+# Wait subscriber to catch up
+$node_u->wait_for_subscription_sync($node_p, $subnames1[0]);
+$node_u->wait_for_subscription_sync($node_p, $subnames1[1]);
```
wait_for_subscription_sync() is used to wait until the initial synchronization is
done, so not suitable here. wait_for_catchup() is more appropriate.
07.
Also, the similar part exists on the pre-existing part (wait_for_subscription_sync
was used there, but I feel this may not be correct). How about unifing them like
attached?
08.
```
+# Verify logical replication works for all databases
+# Insert rows on node P
+$node_p->safe_psql('postgres', "INSERT INTO tbl1 VALUES('first row')");
```
This is confusing because 'fourth row' is inserted to $db1 just after it. How
about the 'row in database postgres' or something?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachment | Content-Type | Size |
---|---|---|
kuroda.diffs | application/octet-stream | 2.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Pyhalov | 2025-03-24 07:07:57 | Re: Add semi-join pushdown to postgres_fdw |
Previous Message | Michael Paquier | 2025-03-24 06:50:46 | Re: Query ID Calculation Fix for DISTINCT / ORDER BY and LIMIT / OFFSET |