RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility.

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

In response to

Browse pgsql-hackers by date

  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