From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Shubham Khanna' <khannashubham1197(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
Subject: | RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. |
Date: | 2025-02-12 07:26:47 |
Message-ID: | OSCPR01MB1496699C56C2E3C170325EB66F5FC2@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! Few comments.
01. pg_createsubscriber.sgml
```
+ <para>
+ Remove all existing publications on the subscriber node before creating
+ a subscription.
+ </para>
+
```
I think this is not accurate. Publications on databases which are not target will
retain. Also, I'm not sure it is important to clarify "before creating a subscription.".
How about: Remove all existing publications from specified databases.
02. main()
```
+ opt.cleanup_existing_publications = false;
```
ISTM initialization is done with the same ordering with CreateSubscriberOptions.
Thus this should be at after recovery_timeout.
03. 040_pg_createsubscriber.pl
```
+$node_p->wait_for_replay_catchup($node_s);
+
+# Verify the existing publications
+my $pub_count_before =
+ $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
+is($pub_count_before, '2',
+ 'two publications created before --cleanup-existing-publications is run');
```
I think no need to declare $pub_count_before. How about:
```
ok( $node_s->poll_query_until(
$db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
'two publications created before --cleanup-existing-publications is run');
```
04. 040_pg_createsubscriber.pl
``` +my $pub_count_after =
+ $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
+is($pub_count_after, '0',
+ 'all publications dropped after --cleanup-existing-publications is run');
+
```
I think no need to declare $pub_count_after. How about:
```
is($node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"), '0',
'all publications dropped after --cleanup-existing-publications is run');
```
05.
According to the document [1], we must do double-quote for each identifiers. But current
patch seems not to do. Below example shows the case when three publications exist.
```
pg_createsubscriber: dropping publications "aaa, bbb, ccc" in database "postgres"
```
I think the output must be `"aaa", "bbb", "ccc"`. This means drop_publication() should be refactored.
[1]: https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES
Best regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Bertrand Drouvot | 2025-02-12 07:44:38 | Re: POC: enable logical decoding when wal_level = 'replica' without a server restart |
Previous Message | Michael Paquier | 2025-02-12 07:23:39 | Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible |