From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Shubham Khanna' <khannashubham1197(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. |
Date: | 2025-02-21 09:36:35 |
Message-ID: | OSCPR01MB14966D845AC77FC46ECEECCE4F5C72@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. Here are comments.
01.
```
/*
* Create the subscriptions, adjust the initial location for logical
* replication and enable the subscriptions. That's the last step for logical
* replication setup. If 'drop_publications' is true, existing publications on
* the subscriber will be dropped before creating new subscriptions.
*/
static void
setup_subscriber(struct LogicalRepInfo *dbinfo, const char *consistent_lsn,
bool drop_publications)
```
Even drop_publications is false, at least one publication would be dropped. The
argument is not correct. I prefer previous name.
02.
```
/* Drop all existing publications if requested */
if (drop_publications)
{
pg_log_info("Dropping all existing publications in database \"%s\"",
dbinfo[i].dbname);
drop_all_publications(conn, dbinfo[i].dbname);
}
/*
* Since the publication was created before the consistent LSN, it is
* available on the subscriber when the physical replica is promoted.
* Remove publications from the subscriber because it has no use.
* Additionally, drop publications existed before this command if
* requested.
*/
drop_publication(conn, &dbinfo[i]);
```
It is quite strange that drop_publication() is called even when drop_all_publications() is called.
03.
Also, I'm not sure pg_log_info() is needed here. _drop_one_publication() outputs dropping publications.
04.
```
/* Helper function to drop a single publication by name. */
static void
_drop_one_publication(PGconn *conn, const char *pubname, const char *dbname)
```
Functions recently added do not have prefix "_". I think it can be removed.
05.
```
pg_log_debug("Executing: %s", query->data);
pg_log_info("Dropping publication %s in database \"%s\"", pubname, dbinfo->dbname);
```
In _drop_one_publication(), dbname is used only for the message. Can we move to pg_log_info()
outside the function and reduce the argument?
06.
Also, please start with lower case.
07.
Also, please preserve the message as much as possible. Previously they are:
```
pg_log_info("dropping publication \"%s\" in database \"%s\"", dbinfo->pubname, dbinfo->dbname);
pg_log_debug("command is: %s", str->data);
```
08.
I feel we must update made_publication.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Bertrand Drouvot | 2025-02-21 09:43:43 | Re: Doc fix of aggressive vacuum threshold for multixact members storage |
Previous Message | Andrew Dunstan | 2025-02-21 09:36:07 | Re: Missing [NO] INDENT flag in XMLSerialize backward parsing |