Re: [PATCH] Add error hints for invalid COPY options

On Wed, 26 Nov 2025 at 11:55, Sugamoto Shinya <shinya34892(at)gmail(dot)com> wrote:
>
>
>
> 2025年11月25日(火) 6:50 Nathan Bossart <nathandbossart(at)gmail(dot)com>:
>>
>> On Mon, Nov 24, 2025 at 11:56:34AM -0800, Masahiko Sawada wrote:
>> > On Sat, Nov 22, 2025 at 8:33 PM Sugamoto Shinya <shinya34892(at)gmail(dot)com> wrote:
>> >> This follows the pattern already used elsewhere in PostgreSQL for providing
>> >> helpful error hints to users.
>> >
>> > Given we have 15 COPY options now, it sounds like a reasonable idea.
>> >
>> > One concern about the patch is that when adding a new COPY option, we
>> > could miss updating valid_copy_options list, resulting in providing a
>> > wrong suggestion. I think we can consider refactoring the COPY option
>> > handling so that we check the given option is a valid name or not by
>> > checking valid_copy_options array and then process the option value.
>>
>> +1. Ideally, folks wouldn't need to update a separate list when adding new
>> options.
>>
>> >> Additionally, this patch corrects a misleading comment for the
>> >> convert_selectively option. The comment stated it was "not-accessible-from-SQL",
>> >> but actualy it has been accessible from SQL due to PostgreSQL's generic option parser.
>> >> The updated comment clarifies that while technically accessible, it's intended for
>> >> internal use and not recommended for end-user use due to potential data loss.
>> >
>> > Hmm, I'm not sure the proposed comment improves the clarification.
>> > It's essentially non-accessible from SQL since we cannot provide a
>> > valid value for convert_selectively from SQL commands.
>>
>> Yeah, I'd leave it alone, at least for this patch.
>>
>> --
>> nathan
>>
>>
>
>
> Thanks for checking my proposal.
>
>
> For the refactoring of the COPY options, it sounds reasonable to me. Let me take that changes in my patch.

Also one little thing:

>+{
>+ {"default", copy_opt_default, true},
>+ {"delimiter", copy_opt_delimiter, true},
>+ {"encoding", copy_opt_encoding, true},
>+ {"escape", copy_opt_escape, true},
>+ {"force_not_null", copy_opt_force_not_null, true},
>+ {"force_null", copy_opt_force_null, true},
>+ {"force_quote", copy_opt_force_quote, true},
>+ {"format", copy_opt_format, true},
>+ {"freeze", copy_opt_freeze, true},
>+ {"header", copy_opt_header, true},
>+ {"log_verbosity", copy_opt_log_verbosity, true},
>+ {"null", copy_opt_null, true},
>+ {"on_error", copy_opt_on_error, true},
>+ {"quote", copy_opt_quote, true},
>+ {"reject_limit", copy_opt_reject_limit, true},
>+ {"convert_selectively", copy_opt_convert_selectively, false},
>+ {NULL, NULL, false}
>+};

Maybe we need one more struct member here, to indicate which options
are valid to be specified by user?

Also, pattern

static const struct {..} array_name[] = ... is not used in PostgreSQL
sources. At least, I do not see any use of such .

--
Best regards,
Kirill Reshke