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

On Sun, Dec 7, 2025 at 6:32 AM Sugamoto Shinya <shinya34892(at)gmail(dot)com> wrote:
>
>
>
> On Thu, Dec 4, 2025 at 4:35 PM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>>
>> On Thu, 4 Dec 2025 at 03:56, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>
>> > The proposed patch requires us to create one function per option. I'm
>> > concerned that it could cause bloating functions and be overkill just
>> > to save changing a separate list. I would suggest starting with
>> > putting a validation check for options at the top of foreach() loop.
>> > When adding a new COPY option in the future, it wouldn't work if we
>> > miss either changing the valid-options list or handling the option,
>> > which seems a good prevention.
>> >
>>
>> Yep, Im +1 on that. "bloating functions" - that's precise wording, I
>> did not know how to explain the same concern.
>>
>>
>> --
>> Best regards,
>> Kirill Reshke
>
>
>
>
>
> Thanks everyone for reviewing my proposal.
>
>
> > The proposed patch requires us to create one function per option. I'm
> > concerned that it could cause bloating functions and be overkill just
> > to save changing a separate list. I would suggest starting with
> > putting a validation check for options at the top of foreach() loop.
> > When adding a new COPY option in the future, it wouldn't work if we
> > miss either changing the valid-options list or handling the option,
> > which seems a good prevention.
>
>
> I completely agree with your feedback. I will proceed with a smaller and
> simpler revision of the changes. The v3 patch was beneficial in that it
> removed duplicated option names for COPY options in the code, but it
> introduced too much refactoring for such a small improvement.
>
> Attached is the new patch. One possible discussion point is that I choose FATAL
> over ERROR at src/backend/commands/copy.c#L816, since reaching that point would
> indicate an implementation problem. Please let me know if there is a better option.

We typically use elog(ERROR) for should-not-happen errors instead of
ereport(ERROR), but I don't think we need the last 'else if' branch
since we have validate_copy_option.

The patch mostly looks good to me. Here are some minor comments:

+#include "utils/elog.h"

I think we don't need to #include elog.h.

---
src/backend/commands/copy.c:819: trailing whitespace.
+

There is unnecessary whitespace.

---
+static const CopyOptionDef valid_copy_options[] = {
+ {"default", true},
+ {"delimiter", true},
+ {"encoding", true},
+ {"escape", true},
+ {"force_not_null", true},
+ {"force_null", true},
+ {"force_quote", true},
+ {"format", true},
+ {"freeze", true},
+ {"header", true},
+ {"log_verbosity", true},
+ {"null", true},
+ {"on_error", true},
+ {"quote", true},
+ {"reject_limit", true},
+ {"convert_selectively", false}, /* internal, undocumented */
+ {NULL, false}
+};

I think we can maintain this list in option name order (not sorted by
suggest_in_hints).

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com