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

On Tue, Mar 18, 2025 at 5:48 AM David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> 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?
>

I have added validation for "all" to address both issues at once.
Additionally, the option parsing is now case-insensitive for better
usability.

The attached patch at [1] contains the suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8RjK8q%2BmWPWPh9K7CeH2tKF31vGn%2BoPV3qY7pdPCmtbjzkg%40mail.gmail.com

Thanks and regards,
Shubham Khanna.