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

On Mon, Mar 17, 2025 at 4:01 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:

> On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote:
>
> I have incorporated the "--remove/-r" parameter in the attached patch,
> as it seems more intuitive and straightforward for users.
> The attached patch contains the latest changes.
>
>
> There were a lot of discussion around the single vs multiple options since
> my
> last review [1] so I'm answering some of the questions here.
>
>

> Regarding the name, my preference is
> --drop since we already have other binaries with similar options
> (pg_receivewal
> and pg_recvlogical have --drop-slot).
>

A short form seems desired here and we cannot use -d/-D. Also, the "act
and quit" nature of the command-like options in those two client
applications leads me to believe that this server application modifier-like
option, which behaves differently than a simple "drop named object and
return", should not have the same naming as those others.

We are not dropping named objects - the wording "Remove all given objects"
is incorrect.

"Remove all objects of the specified type from specified databases on the
target server."

"Multiple object types can be specified by writing multiple --remove
switches." (accepting switches instead of options pending bulk change)

More changes of this sort are needed.

>
> - drop_publication(conn, &dbinfo[i]);
> + if (dbinfos.remove_objects & OBJECT_PUBLICATIONS)
> + drop_all_publications(conn, dbinfo);
> + else
> + drop_publication(conn, dbinfo, dbinfo->pubname);
>
> At first glance, I didn't like this change. You inform dbinfo->pubname as
> a 3rd
> parameter but the 2nd parameter is dbinfo. After reading
> drop_all_publication(), I realized that's the cause for this change. Is
> there a
> better way to do it?
>

I had the same impression. I'm inclined to accept it as-is and let whoever
writes the next --remove object type implementation deal with cleaning it
up. This is clear enough when talking only about publications since
whether you just remove the one or all of them the special one we created
goes away.

+ pg_log_error("wrong remove object is specified");
> + pg_log_error_hint("Only \"publications\" can be removed.");
> + exit(1);
> The main message sounds strange. Did you mean 'wrong object is specified'?
>

Error: invalid object type "%s" specified for --remove
Hint: The valid options are: "publications"

(yes, plural for a single option is "wrong", but extensible...)

If we want to just give the DBA the choice to say "all" now we could solve
two annoyances at once.

The valid options are: "all", "publications"

Should we be accepting these case-insensitively?

David J.