From bb3ece5bb89b1aa265c77a17735d4a206184dabf Mon Sep 17 00:00:00 2001 From: Shinya Sugamoto Date: Sat, 22 Nov 2025 22:03:21 +0900 Subject: [PATCH] Refactor COPY option handling with table-driven approach Refactor ProcessCopyOptions() to use a table-driven approach for option handling. This provides a single source of truth for valid COPY options - when adding a new option, simply add an entry to the copy_options[] table and implement its processor function. Also add error hints for invalid COPY options using ClosestMatch to suggest similar valid option names, and list all valid values for options with few choices (format, on_error, log_verbosity). --- contrib/file_fdw/expected/file_fdw.out | 3 + src/backend/commands/copy.c | 448 ++++++++++++++++--------- src/test/regress/expected/copy2.out | 27 ++ src/test/regress/sql/copy2.sql | 7 + 4 files changed, 323 insertions(+), 162 deletions(-) diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out index 5121e27dce5..bf953c62a49 100644 --- a/contrib/file_fdw/expected/file_fdw.out +++ b/contrib/file_fdw/expected/file_fdw.out @@ -54,6 +54,7 @@ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS ("a=b" 'true'); -- ERROR ERROR: invalid option name "a=b": must not contain "=" CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR ERROR: COPY format "xml" not recognized +HINT: Valid formats are "binary", "csv", and "text". CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR ERROR: COPY QUOTE requires CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR @@ -96,10 +97,12 @@ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null ' ERROR: COPY null representation cannot use newline or carriage return CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (on_error 'unsupported'); -- ERROR ERROR: COPY ON_ERROR "unsupported" not recognized +HINT: Valid values are "ignore" and "stop". CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', on_error 'ignore'); -- ERROR ERROR: only ON_ERROR STOP is allowed in BINARY mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (log_verbosity 'unsupported'); -- ERROR ERROR: COPY LOG_VERBOSITY "unsupported" not recognized +HINT: Valid values are "default", "silent", and "verbose". CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (reject_limit '1'); -- ERROR ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (on_error 'ignore', reject_limit '0'); -- ERROR diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 28e878c3688..b5ffb3c1062 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -38,6 +38,7 @@ #include "utils/lsyscache.h" #include "utils/rel.h" #include "utils/rls.h" +#include "utils/varlena.h" /* * DoCopy executes the SQL COPY statement @@ -467,6 +468,7 @@ defGetCopyOnErrorChoice(DefElem *def, ParseState *pstate, bool is_from) (errcode(ERRCODE_INVALID_PARAMETER_VALUE), /*- translator: first %s is the name of a COPY option, e.g. ON_ERROR */ errmsg("COPY %s \"%s\" not recognized", "ON_ERROR", sval), + errhint("Valid values are \"%s\" and \"%s\".", "ignore", "stop"), parser_errposition(pstate, def->location))); return COPY_ON_ERROR_STOP; /* keep compiler quiet */ } @@ -525,10 +527,259 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate) (errcode(ERRCODE_INVALID_PARAMETER_VALUE), /*- translator: first %s is the name of a COPY option, e.g. ON_ERROR */ errmsg("COPY %s \"%s\" not recognized", "LOG_VERBOSITY", sval), + errhint("Valid values are \"%s\", \"%s\", and \"%s\".", "default", "silent", "verbose"), parser_errposition(pstate, def->location))); return COPY_LOG_VERBOSITY_DEFAULT; /* keep compiler quiet */ } +/* + * Context structure for COPY option processing. + */ +typedef struct CopyOptionContext +{ + ParseState *pstate; + CopyFormatOptions *opts_out; + bool is_from; + bool format_specified; + bool freeze_specified; + bool header_specified; + bool on_error_specified; + bool log_verbosity_specified; + bool reject_limit_specified; +} CopyOptionContext; + +/* Individual option processors */ + +static void +copy_opt_format(CopyOptionContext *ctx, DefElem *defel) +{ + char *fmt = defGetString(defel); + + if (ctx->format_specified) + errorConflictingDefElem(defel, ctx->pstate); + ctx->format_specified = true; + + if (strcmp(fmt, "text") == 0) + /* default format */ ; + else if (strcmp(fmt, "csv") == 0) + ctx->opts_out->csv_mode = true; + else if (strcmp(fmt, "binary") == 0) + ctx->opts_out->binary = true; + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY format \"%s\" not recognized", fmt), + errhint("Valid formats are \"%s\", \"%s\", and \"%s\".", + "binary", "csv", "text"), + parser_errposition(ctx->pstate, defel->location))); +} + +static void +copy_opt_freeze(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->freeze_specified) + errorConflictingDefElem(defel, ctx->pstate); + ctx->freeze_specified = true; + ctx->opts_out->freeze = defGetBoolean(defel); +} + +static void +copy_opt_delimiter(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->delim) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->delim = defGetString(defel); +} + +static void +copy_opt_null(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->null_print) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->null_print = defGetString(defel); +} + +static void +copy_opt_default(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->default_print) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->default_print = defGetString(defel); +} + +static void +copy_opt_header(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->header_specified) + errorConflictingDefElem(defel, ctx->pstate); + ctx->header_specified = true; + ctx->opts_out->header_line = defGetCopyHeaderOption(defel, ctx->is_from); +} + +static void +copy_opt_quote(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->quote) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->quote = defGetString(defel); +} + +static void +copy_opt_escape(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->escape) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->escape = defGetString(defel); +} + +static void +copy_opt_force_quote(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->force_quote || ctx->opts_out->force_quote_all) + errorConflictingDefElem(defel, ctx->pstate); + if (defel->arg && IsA(defel->arg, A_Star)) + ctx->opts_out->force_quote_all = true; + else if (defel->arg && IsA(defel->arg, List)) + ctx->opts_out->force_quote = castNode(List, defel->arg); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("argument to option \"%s\" must be a list of column names", + defel->defname), + parser_errposition(ctx->pstate, defel->location))); +} + +static void +copy_opt_force_not_null(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->force_notnull || ctx->opts_out->force_notnull_all) + errorConflictingDefElem(defel, ctx->pstate); + if (defel->arg && IsA(defel->arg, A_Star)) + ctx->opts_out->force_notnull_all = true; + else if (defel->arg && IsA(defel->arg, List)) + ctx->opts_out->force_notnull = castNode(List, defel->arg); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("argument to option \"%s\" must be a list of column names", + defel->defname), + parser_errposition(ctx->pstate, defel->location))); +} + +static void +copy_opt_force_null(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->force_null || ctx->opts_out->force_null_all) + errorConflictingDefElem(defel, ctx->pstate); + if (defel->arg && IsA(defel->arg, A_Star)) + ctx->opts_out->force_null_all = true; + else if (defel->arg && IsA(defel->arg, List)) + ctx->opts_out->force_null = castNode(List, defel->arg); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("argument to option \"%s\" must be a list of column names", + defel->defname), + parser_errposition(ctx->pstate, defel->location))); +} + +static void +copy_opt_convert_selectively(CopyOptionContext *ctx, DefElem *defel) +{ + /* + * Undocumented, not-accessible-from-SQL option: convert only the + * named columns to binary form, storing the rest as NULLs. It's + * allowed for the column list to be NIL. + */ + if (ctx->opts_out->convert_selectively) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->convert_selectively = true; + if (defel->arg == NULL || IsA(defel->arg, List)) + ctx->opts_out->convert_select = castNode(List, defel->arg); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("argument to option \"%s\" must be a list of column names", + defel->defname), + parser_errposition(ctx->pstate, defel->location))); +} + +static void +copy_opt_encoding(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->opts_out->file_encoding >= 0) + errorConflictingDefElem(defel, ctx->pstate); + ctx->opts_out->file_encoding = pg_char_to_encoding(defGetString(defel)); + if (ctx->opts_out->file_encoding < 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("argument to option \"%s\" must be a valid encoding name", + defel->defname), + parser_errposition(ctx->pstate, defel->location))); +} + +static void +copy_opt_on_error(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->on_error_specified) + errorConflictingDefElem(defel, ctx->pstate); + ctx->on_error_specified = true; + ctx->opts_out->on_error = defGetCopyOnErrorChoice(defel, ctx->pstate, + ctx->is_from); +} + +static void +copy_opt_log_verbosity(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->log_verbosity_specified) + errorConflictingDefElem(defel, ctx->pstate); + ctx->log_verbosity_specified = true; + ctx->opts_out->log_verbosity = defGetCopyLogVerbosityChoice(defel, + ctx->pstate); +} + +static void +copy_opt_reject_limit(CopyOptionContext *ctx, DefElem *defel) +{ + if (ctx->reject_limit_specified) + errorConflictingDefElem(defel, ctx->pstate); + ctx->reject_limit_specified = true; + ctx->opts_out->reject_limit = defGetCopyRejectLimitOption(defel); +} + +/* + * Table of all valid COPY options. + * + * This is the single source of truth for valid COPY options. When adding a + * new option, add an entry here and implement its processor function above. + */ +static const struct +{ + const char *name; + void (*processor) (CopyOptionContext *ctx, DefElem *defel); + bool suggest_in_hints; +} copy_options[] = + +{ + {"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} +}; + /* * Process the statement option list for COPY. * @@ -551,12 +802,7 @@ ProcessCopyOptions(ParseState *pstate, bool is_from, List *options) { - bool format_specified = false; - bool freeze_specified = false; - bool header_specified = false; - bool on_error_specified = false; - bool log_verbosity_specified = false; - bool reject_limit_specified = false; + CopyOptionContext ctx; ListCell *option; /* Support external use for option sanity checking */ @@ -565,177 +811,55 @@ ProcessCopyOptions(ParseState *pstate, opts_out->file_encoding = -1; + /* Initialize option processing context */ + memset(&ctx, 0, sizeof(ctx)); + ctx.pstate = pstate; + ctx.opts_out = opts_out; + ctx.is_from = is_from; + /* Extract options from the statement node tree */ foreach(option, options) { DefElem *defel = lfirst_node(DefElem, option); + bool found = false; - if (strcmp(defel->defname, "format") == 0) - { - char *fmt = defGetString(defel); - - if (format_specified) - errorConflictingDefElem(defel, pstate); - format_specified = true; - if (strcmp(fmt, "text") == 0) - /* default format */ ; - else if (strcmp(fmt, "csv") == 0) - opts_out->csv_mode = true; - else if (strcmp(fmt, "binary") == 0) - opts_out->binary = true; - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("COPY format \"%s\" not recognized", fmt), - parser_errposition(pstate, defel->location))); - } - else if (strcmp(defel->defname, "freeze") == 0) - { - if (freeze_specified) - errorConflictingDefElem(defel, pstate); - freeze_specified = true; - opts_out->freeze = defGetBoolean(defel); - } - else if (strcmp(defel->defname, "delimiter") == 0) - { - if (opts_out->delim) - errorConflictingDefElem(defel, pstate); - opts_out->delim = defGetString(defel); - } - else if (strcmp(defel->defname, "null") == 0) - { - if (opts_out->null_print) - errorConflictingDefElem(defel, pstate); - opts_out->null_print = defGetString(defel); - } - else if (strcmp(defel->defname, "default") == 0) - { - if (opts_out->default_print) - errorConflictingDefElem(defel, pstate); - opts_out->default_print = defGetString(defel); - } - else if (strcmp(defel->defname, "header") == 0) - { - if (header_specified) - errorConflictingDefElem(defel, pstate); - header_specified = true; - opts_out->header_line = defGetCopyHeaderOption(defel, is_from); - } - else if (strcmp(defel->defname, "quote") == 0) - { - if (opts_out->quote) - errorConflictingDefElem(defel, pstate); - opts_out->quote = defGetString(defel); - } - else if (strcmp(defel->defname, "escape") == 0) - { - if (opts_out->escape) - errorConflictingDefElem(defel, pstate); - opts_out->escape = defGetString(defel); - } - else if (strcmp(defel->defname, "force_quote") == 0) + /* Look up the option in copy_options table */ + for (int i = 0; copy_options[i].name != NULL; i++) { - if (opts_out->force_quote || opts_out->force_quote_all) - errorConflictingDefElem(defel, pstate); - if (defel->arg && IsA(defel->arg, A_Star)) - opts_out->force_quote_all = true; - else if (defel->arg && IsA(defel->arg, List)) - opts_out->force_quote = castNode(List, defel->arg); - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("argument to option \"%s\" must be a list of column names", - defel->defname), - parser_errposition(pstate, defel->location))); - } - else if (strcmp(defel->defname, "force_not_null") == 0) - { - if (opts_out->force_notnull || opts_out->force_notnull_all) - errorConflictingDefElem(defel, pstate); - if (defel->arg && IsA(defel->arg, A_Star)) - opts_out->force_notnull_all = true; - else if (defel->arg && IsA(defel->arg, List)) - opts_out->force_notnull = castNode(List, defel->arg); - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("argument to option \"%s\" must be a list of column names", - defel->defname), - parser_errposition(pstate, defel->location))); - } - else if (strcmp(defel->defname, "force_null") == 0) - { - if (opts_out->force_null || opts_out->force_null_all) - errorConflictingDefElem(defel, pstate); - if (defel->arg && IsA(defel->arg, A_Star)) - opts_out->force_null_all = true; - else if (defel->arg && IsA(defel->arg, List)) - opts_out->force_null = castNode(List, defel->arg); - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("argument to option \"%s\" must be a list of column names", - defel->defname), - parser_errposition(pstate, defel->location))); + if (strcmp(copy_options[i].name, defel->defname) == 0) + { + copy_options[i].processor(&ctx, defel); + found = true; + break; + } } - else if (strcmp(defel->defname, "convert_selectively") == 0) + + if (!found) { /* - * Undocumented, not-accessible-from-SQL option: convert only the - * named columns to binary form, storing the rest as NULLs. It's - * allowed for the column list to be NIL. + * When an unknown option is specified, complain about it. + * Provide a hint with a valid option that looks similar, if there + * is one. */ - if (opts_out->convert_selectively) - errorConflictingDefElem(defel, pstate); - opts_out->convert_selectively = true; - if (defel->arg == NULL || IsA(defel->arg, List)) - opts_out->convert_select = castNode(List, defel->arg); - else - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("argument to option \"%s\" must be a list of column names", - defel->defname), - parser_errposition(pstate, defel->location))); - } - else if (strcmp(defel->defname, "encoding") == 0) - { - if (opts_out->file_encoding >= 0) - errorConflictingDefElem(defel, pstate); - opts_out->file_encoding = pg_char_to_encoding(defGetString(defel)); - if (opts_out->file_encoding < 0) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("argument to option \"%s\" must be a valid encoding name", - defel->defname), - parser_errposition(pstate, defel->location))); - } - else if (strcmp(defel->defname, "on_error") == 0) - { - if (on_error_specified) - errorConflictingDefElem(defel, pstate); - on_error_specified = true; - opts_out->on_error = defGetCopyOnErrorChoice(defel, pstate, is_from); - } - else if (strcmp(defel->defname, "log_verbosity") == 0) - { - if (log_verbosity_specified) - errorConflictingDefElem(defel, pstate); - log_verbosity_specified = true; - opts_out->log_verbosity = defGetCopyLogVerbosityChoice(defel, pstate); - } - else if (strcmp(defel->defname, "reject_limit") == 0) - { - if (reject_limit_specified) - errorConflictingDefElem(defel, pstate); - reject_limit_specified = true; - opts_out->reject_limit = defGetCopyRejectLimitOption(defel); - } - else + ClosestMatchState match_state; + const char *closest_match; + + initClosestMatch(&match_state, defel->defname, 4); + for (int i = 0; copy_options[i].name != NULL; i++) + { + if (copy_options[i].suggest_in_hints) + updateClosestMatch(&match_state, copy_options[i].name); + } + + closest_match = getClosestMatch(&match_state); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("option \"%s\" not recognized", defel->defname), + closest_match ? + errhint("Perhaps you meant \"%s\".", closest_match) : 0, parser_errposition(pstate, defel->location))); + } } /* diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out index f3fdce23459..a4b4e33248f 100644 --- a/src/test/regress/expected/copy2.out +++ b/src/test/regress/expected/copy2.out @@ -96,6 +96,7 @@ COPY x from stdin (on_error unsupported); ERROR: COPY ON_ERROR "unsupported" not recognized LINE 1: COPY x from stdin (on_error unsupported); ^ +HINT: Valid values are "ignore" and "stop". COPY x from stdin (format TEXT, force_quote(a)); ERROR: COPY FORCE_QUOTE requires CSV mode COPY x from stdin (format TEXT, force_quote *); @@ -128,6 +129,7 @@ COPY x from stdin (log_verbosity unsupported); ERROR: COPY LOG_VERBOSITY "unsupported" not recognized LINE 1: COPY x from stdin (log_verbosity unsupported); ^ +HINT: Valid values are "default", "silent", and "verbose". COPY x from stdin with (reject_limit 1); ERROR: COPY REJECT_LIMIT requires ON_ERROR to be set to IGNORE COPY x from stdin with (on_error ignore, reject_limit 0); @@ -138,6 +140,31 @@ COPY x from stdin with (header 2.5); ERROR: header requires a Boolean value, a non-negative integer, or the string "match" COPY x to stdout with (header 2); ERROR: cannot use multi-line header in COPY TO +-- test error hints for invalid COPY options +COPY x from stdin (foramt CSV); -- error, suggests "format" +ERROR: option "foramt" not recognized +LINE 1: COPY x from stdin (foramt CSV); + ^ +HINT: Perhaps you meant "format". +COPY x from stdin (completely_invalid_option 'value'); -- error, no suggestion +ERROR: option "completely_invalid_option" not recognized +LINE 1: COPY x from stdin (completely_invalid_option 'value'); + ^ +COPY x from stdin (format cvs); -- error, lists valid formats +ERROR: COPY format "cvs" not recognized +LINE 1: COPY x from stdin (format cvs); + ^ +HINT: Valid formats are "binary", "csv", and "text". +COPY x from stdin (format CSV, on_error ignor); -- error, lists valid on_error values +ERROR: COPY ON_ERROR "ignor" not recognized +LINE 1: COPY x from stdin (format CSV, on_error ignor); + ^ +HINT: Valid values are "ignore" and "stop". +COPY x from stdin (format CSV, log_verbosity verbos); -- error, lists valid log_verbosity values +ERROR: COPY LOG_VERBOSITY "verbos" not recognized +LINE 1: COPY x from stdin (format CSV, log_verbosity verbos); + ^ +HINT: Valid values are "default", "silent", and "verbose". -- too many columns in column list: should fail COPY x (a, b, c, d, e, d, c) from stdin; ERROR: column "d" specified more than once diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql index cef45868db5..b8c9d2439ef 100644 --- a/src/test/regress/sql/copy2.sql +++ b/src/test/regress/sql/copy2.sql @@ -94,6 +94,13 @@ COPY x from stdin with (header -1); COPY x from stdin with (header 2.5); COPY x to stdout with (header 2); +-- test error hints for invalid COPY options +COPY x from stdin (foramt CSV); -- error, suggests "format" +COPY x from stdin (completely_invalid_option 'value'); -- error, no suggestion +COPY x from stdin (format cvs); -- error, lists valid formats +COPY x from stdin (format CSV, on_error ignor); -- error, lists valid on_error values +COPY x from stdin (format CSV, log_verbosity verbos); -- error, lists valid log_verbosity values + -- too many columns in column list: should fail COPY x (a, b, c, d, e, d, c) from stdin; -- 2.50.1 (Apple Git-155)