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>, 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

In response to

Browse pgsql-hackers by date

  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