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

On Fri, Nov 28, 2025 at 2:59 AM Kirill Reshke <reshkekirill(at)gmail(dot)com>
wrote:

> 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
>

Thanks for checking my proposal.

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

If you don't mind, I would like to make a separate patch for fixing the
"convert_selectively"
option and focus on refactoring error handling here because I tend to feel
we should
separate refactoring changes and non-backward compatible changes into
different commits.
After this patch gets merged, I'll make another thread to discuss whether
we should block
unexpected "convert_selectively" use or not.

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

I saw several places that use that sort of style, for example
src/backend/utils/adt/encode.c:836
and src/backend/access/heap/heapam.c:122, but you seems to be more or less
correct since
usually we define types explicitly like src/backend/foreign/foreign.c:576
and src/backend/backup/basebackup.c:191.
I updated my patch by defining a new type `CopyCoptionDef`.

Also, I added improvements to helper functions like defGet**. I just
removed and unified those
into corresponding proceeOption** functions.

Regards,