On Tue, Mar 18, 2025 at 5:48 AM David G. Johnston
<david.g.johns...@gmail.com> wrote:
>
> On Mon, Mar 17, 2025 at 4:01 PM Euler Taveira <eu...@eulerto.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.
