On Fri, Nov 28, 2025 at 2:59 AM Kirill Reshke <[email protected]> wrote:
> On Wed, 26 Nov 2025 at 11:55, Sugamoto Shinya <[email protected]> > wrote: > > > > > > > > 2025年11月25日(火) 6:50 Nathan Bossart <[email protected]>: > >> > >> On Mon, Nov 24, 2025 at 11:56:34AM -0800, Masahiko Sawada wrote: > >> > On Sat, Nov 22, 2025 at 8:33 PM Sugamoto Shinya < > [email protected]> 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,
v3-0001-Refactor-COPY-option-handling.patch
Description: Binary data
