On Wed, Mar 19, 2025 at 12:44 PM Zhijie Hou (Fujitsu) <houzj.f...@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.
v22-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
