RE: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.

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

In response to

Responses

Browse pgsql-hackers by date

  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