On Wed, Mar 19, 2025 at 12:44 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Wed, Mar 19, 2025 at 2:55 PM Shubham Khanna wrote:
> >
> > I have merged the changes and prepared the latest patch. The attached
> > patch contains the suggested changes.
>
> Thanks for updating the patch. Here are few comments:
>
Thank you for the suggestions. I agree with the changes you have attached.
> 1.
> pg_log_error("object type \"%s\" is specified more than once for --remove",
optarg);
> exit(1);
>
> Consider using pg_fatal for simplicity.
>
Replacing pg_log_error with pg_fatal makes the error handling more consistent.
>
> 2.
>
> + /* Fetch all publication names */
> + res = PQexec(conn, "SELECT pubname FROM pg_catalog.pg_publication;");
> + if (PQresultStatus(res) != PGRES_TUPLES_OK)
> + {
> + pg_log_error("could not obtain publication information: %s",
> + PQresultErrorMessage(res));
> + PQclear(res);
> + disconnect_database(conn, false);
> + return;
> + }
>
> I think we should exit here for consistency, as performed in similar cases.
>
Exiting on failure when fetching publication names improves
consistency with similar cases.
>
> 3.
>
> + pg_log_info("dropped all publications in database \"%s\"", dbinfo->dbname);
>
> This message may be misleading if some publications were not dropped
> successfully, as drop_publication does not exit on a drop failure.
>
Updating the log message to reflect partial drop failures avoids
misleading information.
> 4.
>
> if (opt.remove_objects.head != NULL)
> {
> for (SimpleStringListCell *cell = opt.remove_objects.head; cell; cell = cell->next)
> {
>
> I think the first null test is redundant.
>
> I have attached a patch with the proposed changes. If you agree with these
> modifications, please merge them.
>
Removing the redundant NULL check simplifies the logic.
I have merged the changes and prepared the latest patch. The attached
patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
