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>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
Cc: 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-19 10:53:30
Message-ID: OSCPR01MB14966A3697FF2F44FC28B3C81F5C52@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!

> I have added a test case for non-dry-run mode to ensure that
> replication slots, subscriptions, and publications work as expected
> when '--all' is specified. Additionally, I have split the test file
> into two parts:
> 1) Command-line sanity checks – Validates the correctness of option parsing.
> 2) '--all' functionality and behavior – Verifies the health of
> subscriptions and ensures that --all specific scenarios, such as
> non-template databases not being subscribed to, are properly handled.
> This should improve test coverage while keeping the structure manageable.

TBH, I feel your change does not separate the test file into the two parts. ISTM
you just added some validation checks and a test how --all option works.
Ashutosh, does it match your suggestion?

Anyway, here are my comments.

01.
```
+ int num_rows;
```

I think num_rows is not needed, we can do like:

```
/* Process the query result */
- num_rows = PQntuples(res);
- for (int i = 0; i < num_rows; i++)
+ for (int i = 0; i < PQntuples(res); i+
```
And
```
/* Error if no databases were found on the source server */
- if (num_rows == 0)
+ if (num_dbs == 0)
```

02.
```
+ opt.all = false;
```

This must be done after initializing recovery_timeout.

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

I do not like the name "S1" because this is the second standby sever
from the node_p.

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.

05.
```
+# Verify replication is working
+$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
+
+$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
+is( $result, qq(first row
+replication test
+second row), "replication successful in $db1");
+
+$node_s1->stop;
# Run pg_createsubscriber on node S. --verbose is used twice
# to show more information.
command_ok(
```

I'm not sure the part is not needed. This have already been checked in other parts, right?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Mahendra Singh Thalor 2025-02-19 11:32:17 Re: Non-text mode for pg_dumpall
Previous Message Jakub Wartak 2025-02-19 10:33:45 Re: pg_upgrade fails for PostGIS custom SRIDs