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>, 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>
Subject: RE: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.
Date: 2025-02-20 11:26:30
Message-ID: OSCPR01MB14966E0AEEE6030573D458035F5C42@OSCPR01MB14966.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Shubham,

Thanks for updating the patch quickly!

> > 04.
> > ```
> > +# Verify that only user databases got subscriptions (not template databases)
> > +my @user_dbs = ($db1, $db2);
> > +foreach my $dbname (@user_dbs)
> > +{
> > + my $sub_exists =
> > + $node_s1->safe_psql($dbname, "SELECT count(*) FROM
> pg_subscription;");
> > + is($sub_exists, '3', "Subscription created successfully for $dbname");
> > +}
> > ```
> >
> > Hmm, what do you want to check here? pg_subscription is a global catalog so
> that
> > very loop will see exactly the same content. Also, 'postgres' is also the user
> database.
> > I feel you must ensure that all three databases (postgres, $db1, and $db2) have
> a
> > subscription here.
> >
>
> Fixed.

My point was that the loop does not have meaning because pg_subscription
is a global one. I and Peter considered changes like [1] is needed. It ensures
that each databases have a subscription.
Note: [1] cannot pass the test as-is because $db1 and $db2 contains special
characters. Please escape appropriately.

Other comments are listed in below.

01.
```
+ <term><option>-a</option></term>
+ <term><option>--all-dbs</option></term>
```

Peter's comment [2] does not say that option name should be changed.
The scope of his comment is only in the .c file.

02.
```
+# Set up node S2 as standby linking to node P
+$node_p->backup('backup_3');
+my $node_s2 = PostgreSQL::Test::Cluster->new('node_s2');
```

I feel $node_s should be renamed to $node_s1, then you can use $node_s2.

Note that this change may not be needed based on the comment [3].
We may have to separate the test file.

[1]:
```
# Verify that only user databases got subscriptions (not template databases)
my @user_dbs = ('postgres', $db1, $db2);
foreach my $dbname (@user_dbs)
{
$result =
$node_s2->safe_psql('postgres',
"SELECT count(*) FROM pg_subscription, pg_database WHERE subdbid = pg_database.oid and datname = '$dbname';");

is($result, '1', "Subscription created successfully for $dbname");
}
```
[2]: https://www.postgresql.org/message-id/CAHut%2BPsatTfk9-F4JBrX_yYE0QGh4wQiTmvS4%3DdnBxcL%3DAK2HA%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAExHW5vmMs5nZ6%3DXcCYAXMJrhVrsW7hOovyg%2BP%2BT9Pkuc7DykA%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nisha Moond 2025-02-20 11:30:48 Conflict detection for multiple_unique_conflicts in logical replication
Previous Message Thomas Munro 2025-02-20 11:22:56 Re: Commitfest app release on Feb 17 with many improvements