public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v1 2/3] Replace magic "one" in tsvector binary receiving=0A= 24+ messages / 4 participants [nested] [flat]
* [PATCH v1 2/3] Replace magic "one" in tsvector binary receiving=0A= @ 2023-09-19 08:40 Denis Erokhin <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Denis Erokhin @ 2023-09-19 08:40 UTC (permalink / raw) ------=_NextPart_000_0099_01D9F2F3.17E4DDF0 Content-Type: application/octet-stream; name="v1-0003-Optimize-memory-allocation-during-tsvector-binary.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="v1-0003-Optimize-memory-allocation-during-tsvector-binary.patch" ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations @ 2025-03-03 19:06 Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Masahiko Sawada @ 2025-03-03 19:06 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers On Sun, Mar 2, 2025 at 4:19 PM Sutou Kouhei <[email protected]> wrote: > > Hi, > > In <[email protected]> > "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 02 Mar 2025 11:27:20 -0500, > Tom Lane <[email protected]> wrote: > > >> While review another thread (Emitting JSON to file using COPY TO), > >> I found the recently committed patches on this thread pass the > >> CopyFormatOptions struct directly rather a pointer of the struct > >> as a function parameter of CopyToGetRoutine and CopyFromGetRoutine. > > > > Coverity is unhappy about that too: > > > > /srv/coverity/git/pgsql-git/postgresql/src/backend/commands/copyto.c: 177 in CopyToGetRoutine() > > 171 .CopyToOneRow = CopyToBinaryOneRow, > > 172 .CopyToEnd = CopyToBinaryEnd, > > 173 }; > > 174 > > 175 /* Return a COPY TO routine for the given options */ > > 176 static const CopyToRoutine * > >>>> CID 1643911: Performance inefficiencies (PASS_BY_VALUE) > >>>> Passing parameter opts of type "CopyFormatOptions" (size 184 bytes) by value, which exceeds the low threshold of 128 bytes. > > 177 CopyToGetRoutine(CopyFormatOptions opts) > > 178 { > > 179 if (opts.csv_mode) > > 180 return &CopyToRoutineCSV; > > > > (and likewise for CopyFromGetRoutine). I realize that these > > functions aren't called often enough for performance to be an > > overriding concern, but it still seems like poor style. > > > >> Then I took a quick look at the newly rebased patch set and > >> found Sutou has already fixed this issue. > > > > +1, except I'd suggest declaring the parameters as > > "const CopyFormatOptions *opts". > > Thanks for pointing out this (and sorry for missing this in > our reviews...)! > > How about the attached patch? > > I'll rebase the v35 patch set after this is fixed. Thank you for reporting the issue and making the patch. I agree with the fix and the patch looks good to me. I've updated the commit message and am going to push, barring any objections. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com Attachments: [application/octet-stream] v2-0001-Refactor-Copy-From-To-GetRoutine-to-use-pass-by-r.patch (2.8K, ../../CAD21AoAwOP7p6LgmkPGqPuJ5KbJPPQsSZsFzwCDguwzr9F677Q@mail.gmail.com/2-v2-0001-Refactor-Copy-From-To-GetRoutine-to-use-pass-by-r.patch) download | inline diff: From c1fb9b2e93920e400a3cd3bcfbe593163cb1b2aa Mon Sep 17 00:00:00 2001 From: Masahiko Sawada <[email protected]> Date: Mon, 3 Mar 2025 10:50:09 -0800 Subject: [PATCH v2] Refactor Copy{From|To}GetRoutine() to use pass-by-reference argument. The change improves efficiency by eliminating unnecessary copying of CopyFormatOptions. The coverity also complained about inefficiencies caused by pass-by-value. Oversight in 7717f6300 and 2e4127b6d. Reported-by: Junwang Zhao <[email protected]> Reported-by: Tom Lane <[email protected]> (per reports from coverity) Author: Sutou Kouhei <[email protected]> Reviewed-by: Junwang Zhao <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Discussion: https://postgr.es/m/CAEG8a3L6YCpPksTQMzjD_CvwDEhW3D_t=5md9BvvdOs5k+TA=Q@mail.gmail.com --- src/backend/commands/copyfrom.c | 8 ++++---- src/backend/commands/copyto.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 198cee2bc48..bcf66f0adf8 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -153,11 +153,11 @@ static const CopyFromRoutine CopyFromRoutineBinary = { /* Return a COPY FROM routine for the given options */ static const CopyFromRoutine * -CopyFromGetRoutine(CopyFormatOptions opts) +CopyFromGetRoutine(const CopyFormatOptions *opts) { - if (opts.csv_mode) + if (opts->csv_mode) return &CopyFromRoutineCSV; - else if (opts.binary) + else if (opts->binary) return &CopyFromRoutineBinary; /* default is text */ @@ -1574,7 +1574,7 @@ BeginCopyFrom(ParseState *pstate, ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options); /* Set the format routine */ - cstate->routine = CopyFromGetRoutine(cstate->opts); + cstate->routine = CopyFromGetRoutine(&cstate->opts); /* Process the target relation */ cstate->rel = rel; diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index 721d29f8e53..84a3f3879a8 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -174,11 +174,11 @@ static const CopyToRoutine CopyToRoutineBinary = { /* Return a COPY TO routine for the given options */ static const CopyToRoutine * -CopyToGetRoutine(CopyFormatOptions opts) +CopyToGetRoutine(const CopyFormatOptions *opts) { - if (opts.csv_mode) + if (opts->csv_mode) return &CopyToRoutineCSV; - else if (opts.binary) + else if (opts->binary) return &CopyToRoutineBinary; /* default is text */ @@ -700,7 +700,7 @@ BeginCopyTo(ParseState *pstate, ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options); /* Set format routine */ - cstate->routine = CopyToGetRoutine(cstate->opts); + cstate->routine = CopyToGetRoutine(&cstate->opts); /* Process the source/target relation or query */ if (rel) -- 2.43.5 ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-05 00:06 ` Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Sutou Kouhei @ 2025-03-05 00:06 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAD21AoAwOP7p6LgmkPGqPuJ5KbJPPQsSZsFzwCDguwzr9F677Q@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 3 Mar 2025 11:06:39 -0800, Masahiko Sawada <[email protected]> wrote: > I agree with the fix and the patch looks good to me. I've updated the > commit message and am going to push, barring any objections. Thanks! I've rebased the patch set. Here is a summary again: > 0001-0003 are for COPY TO and 0004-0007 are for COPY FROM. > > For COPY TO: > > 0001: Add support for adding custom COPY TO format. This > uses tablesample like handler approach. We've discussed > other approaches such as USING+CREATE XXX approach but it > seems that other approaches are overkill for this case. > > See also: https://www.postgresql.org/message-id/flat/d838025aceeb19c9ff1db702fa55cabf%40postgrespro.ru#caca279... > > 0002: Export CopyToStateData to implement custom COPY TO > format as extension. > > 0003: Export a function and add a private space to > CopyToStateData to implement custom COPY TO format as > extension. > > We may want to squash 0002 and 0003 but splitting them will > be easy to review. Because 0002 just moves existing codes > (with some rename) and 0003 just adds some codes. If we > squash 0002 and 0003, moving and adding are mixed. > > For COPY FROM: > > 0004: This is COPY FROM version of 0001. > > 0005: 0002 has COPY_ prefix -> COPY_DEST_ prefix change for > enum CopyDest. This is similar change for enum CopySource. > > 0006: This is COPY FROM version of 0003. > > 0007: This is for easy to implement "ON_ERROR stop" and > "LOG_VERBOSITY verbose" in extension. > > We may want to squash 0005-0007 like for 0002-0003. Thanks, -- kou Attachments: [text/x-patch] v36-0001-Add-support-for-adding-custom-COPY-TO-format.patch (19.0K, ../../[email protected]/2-v36-0001-Add-support-for-adding-custom-COPY-TO-format.patch) download | inline diff: From 479c601915b30e4f67e5ed047c6fbf3e35702ec6 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Mon, 25 Nov 2024 12:19:15 +0900 Subject: [PATCH v36 1/7] Add support for adding custom COPY TO format This uses the handler approach like tablesample. The approach creates an internal function that returns an internal struct. In this case, a COPY TO handler returns a CopyToRoutine. This also add a test module for custom COPY TO handler. --- src/backend/commands/copy.c | 79 ++++++++++++++++--- src/backend/commands/copyto.c | 28 ++++++- src/backend/nodes/Makefile | 1 + src/backend/nodes/gen_node_support.pl | 2 + src/backend/utils/adt/pseudotypes.c | 1 + src/include/catalog/pg_proc.dat | 6 ++ src/include/catalog/pg_type.dat | 6 ++ src/include/commands/copy.h | 1 + src/include/commands/copyapi.h | 2 + src/include/nodes/meson.build | 1 + src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_copy_format/.gitignore | 4 + src/test/modules/test_copy_format/Makefile | 23 ++++++ .../expected/test_copy_format.out | 17 ++++ src/test/modules/test_copy_format/meson.build | 33 ++++++++ .../test_copy_format/sql/test_copy_format.sql | 5 ++ .../test_copy_format--1.0.sql | 8 ++ .../test_copy_format/test_copy_format.c | 63 +++++++++++++++ .../test_copy_format/test_copy_format.control | 4 + 20 files changed, 269 insertions(+), 17 deletions(-) mode change 100644 => 100755 src/backend/nodes/gen_node_support.pl create mode 100644 src/test/modules/test_copy_format/.gitignore create mode 100644 src/test/modules/test_copy_format/Makefile create mode 100644 src/test/modules/test_copy_format/expected/test_copy_format.out create mode 100644 src/test/modules/test_copy_format/meson.build create mode 100644 src/test/modules/test_copy_format/sql/test_copy_format.sql create mode 100644 src/test/modules/test_copy_format/test_copy_format--1.0.sql create mode 100644 src/test/modules/test_copy_format/test_copy_format.c create mode 100644 src/test/modules/test_copy_format/test_copy_format.control diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index cfca9d9dc29..8d94bc313eb 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -32,6 +32,7 @@ #include "parser/parse_coerce.h" #include "parser/parse_collate.h" #include "parser/parse_expr.h" +#include "parser/parse_func.h" #include "parser/parse_relation.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -476,6 +477,70 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate) return COPY_LOG_VERBOSITY_DEFAULT; /* keep compiler quiet */ } +/* + * Process the "format" option. + * + * This function checks whether the option value is a built-in format such as + * "text" and "csv" or not. If the option value isn't a built-in format, this + * function finds a COPY format handler that returns a CopyToRoutine (for + * is_from == false). If no COPY format handler is found, this function + * reports an error. + */ +static void +ProcessCopyOptionFormat(ParseState *pstate, + CopyFormatOptions *opts_out, + bool is_from, + DefElem *defel) +{ + char *format; + Oid funcargtypes[1]; + Oid handlerOid = InvalidOid; + + format = defGetString(defel); + + opts_out->csv_mode = false; + opts_out->binary = false; + /* built-in formats */ + if (strcmp(format, "text") == 0) + { + /* "csv_mode == false && binary == false" means "text" */ + return; + } + else if (strcmp(format, "csv") == 0) + { + opts_out->csv_mode = true; + return; + } + else if (strcmp(format, "binary") == 0) + { + opts_out->binary = true; + return; + } + + /* custom format */ + if (!is_from) + { + funcargtypes[0] = INTERNALOID; + handlerOid = LookupFuncName(list_make1(makeString(format)), 1, + funcargtypes, true); + } + if (!OidIsValid(handlerOid)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY format \"%s\" not recognized", format), + parser_errposition(pstate, defel->location))); + + /* check that handler has correct return type */ + if (get_func_rettype(handlerOid) != COPY_HANDLEROID) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("function %s must return type %s", + format, "copy_handler"), + parser_errposition(pstate, defel->location))); + + opts_out->handler = handlerOid; +} + /* * Process the statement option list for COPY. * @@ -519,22 +584,10 @@ ProcessCopyOptions(ParseState *pstate, 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))); + ProcessCopyOptionFormat(pstate, opts_out, is_from, defel); } else if (strcmp(defel->defname, "freeze") == 0) { diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index 84a3f3879a8..fce8501dc30 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -150,6 +150,7 @@ static void CopySendInt16(CopyToState cstate, int16 val); /* text format */ static const CopyToRoutine CopyToRoutineText = { + .type = T_CopyToRoutine, .CopyToStart = CopyToTextLikeStart, .CopyToOutFunc = CopyToTextLikeOutFunc, .CopyToOneRow = CopyToTextOneRow, @@ -158,6 +159,7 @@ static const CopyToRoutine CopyToRoutineText = { /* CSV format */ static const CopyToRoutine CopyToRoutineCSV = { + .type = T_CopyToRoutine, .CopyToStart = CopyToTextLikeStart, .CopyToOutFunc = CopyToTextLikeOutFunc, .CopyToOneRow = CopyToCSVOneRow, @@ -166,6 +168,7 @@ static const CopyToRoutine CopyToRoutineCSV = { /* binary format */ static const CopyToRoutine CopyToRoutineBinary = { + .type = T_CopyToRoutine, .CopyToStart = CopyToBinaryStart, .CopyToOutFunc = CopyToBinaryOutFunc, .CopyToOneRow = CopyToBinaryOneRow, @@ -176,13 +179,30 @@ static const CopyToRoutine CopyToRoutineBinary = { static const CopyToRoutine * CopyToGetRoutine(const CopyFormatOptions *opts) { - if (opts->csv_mode) + if (OidIsValid(opts->handler)) + { + Datum datum; + Node *routine; + + datum = OidFunctionCall1(opts->handler, BoolGetDatum(false)); + routine = (Node *) DatumGetPointer(datum); + if (routine == NULL || !IsA(routine, CopyToRoutine)) + ereport( + ERROR, + (errcode( + ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY handler function " + "%u did not return " + "CopyToRoutine struct", + opts->handler))); + return castNode(CopyToRoutine, routine); + } + else if (opts->csv_mode) return &CopyToRoutineCSV; else if (opts->binary) return &CopyToRoutineBinary; - - /* default is text */ - return &CopyToRoutineText; + else + return &CopyToRoutineText; } /* Implementation of the start callback for text and CSV formats */ diff --git a/src/backend/nodes/Makefile b/src/backend/nodes/Makefile index 77ddb9ca53f..dc6c1087361 100644 --- a/src/backend/nodes/Makefile +++ b/src/backend/nodes/Makefile @@ -50,6 +50,7 @@ node_headers = \ access/sdir.h \ access/tableam.h \ access/tsmapi.h \ + commands/copyapi.h \ commands/event_trigger.h \ commands/trigger.h \ executor/tuptable.h \ diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl old mode 100644 new mode 100755 index 1a657f7e0ae..fb90635a245 --- a/src/backend/nodes/gen_node_support.pl +++ b/src/backend/nodes/gen_node_support.pl @@ -62,6 +62,7 @@ my @all_input_files = qw( access/sdir.h access/tableam.h access/tsmapi.h + commands/copyapi.h commands/event_trigger.h commands/trigger.h executor/tuptable.h @@ -86,6 +87,7 @@ my @nodetag_only_files = qw( access/sdir.h access/tableam.h access/tsmapi.h + commands/copyapi.h commands/event_trigger.h commands/trigger.h executor/tuptable.h diff --git a/src/backend/utils/adt/pseudotypes.c b/src/backend/utils/adt/pseudotypes.c index 317a1f2b282..f2ebc21ca56 100644 --- a/src/backend/utils/adt/pseudotypes.c +++ b/src/backend/utils/adt/pseudotypes.c @@ -370,6 +370,7 @@ PSEUDOTYPE_DUMMY_IO_FUNCS(fdw_handler); PSEUDOTYPE_DUMMY_IO_FUNCS(table_am_handler); PSEUDOTYPE_DUMMY_IO_FUNCS(index_am_handler); PSEUDOTYPE_DUMMY_IO_FUNCS(tsm_handler); +PSEUDOTYPE_DUMMY_IO_FUNCS(copy_handler); PSEUDOTYPE_DUMMY_IO_FUNCS(internal); PSEUDOTYPE_DUMMY_IO_FUNCS(anyelement); PSEUDOTYPE_DUMMY_IO_FUNCS(anynonarray); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index cd9422d0bac..9e7737168c4 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -7809,6 +7809,12 @@ { oid => '3312', descr => 'I/O', proname => 'tsm_handler_out', prorettype => 'cstring', proargtypes => 'tsm_handler', prosrc => 'tsm_handler_out' }, +{ oid => '8753', descr => 'I/O', + proname => 'copy_handler_in', proisstrict => 'f', prorettype => 'copy_handler', + proargtypes => 'cstring', prosrc => 'copy_handler_in' }, +{ oid => '8754', descr => 'I/O', + proname => 'copy_handler_out', prorettype => 'cstring', + proargtypes => 'copy_handler', prosrc => 'copy_handler_out' }, { oid => '267', descr => 'I/O', proname => 'table_am_handler_in', proisstrict => 'f', prorettype => 'table_am_handler', proargtypes => 'cstring', diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat index 6dca77e0a22..340e0cd0a8d 100644 --- a/src/include/catalog/pg_type.dat +++ b/src/include/catalog/pg_type.dat @@ -633,6 +633,12 @@ typcategory => 'P', typinput => 'tsm_handler_in', typoutput => 'tsm_handler_out', typreceive => '-', typsend => '-', typalign => 'i' }, +{ oid => '8752', + descr => 'pseudo-type for the result of a copy to method function', + typname => 'copy_handler', typlen => '4', typbyval => 't', typtype => 'p', + typcategory => 'P', typinput => 'copy_handler_in', + typoutput => 'copy_handler_out', typreceive => '-', typsend => '-', + typalign => 'i' }, { oid => '269', descr => 'pseudo-type for the result of a table AM handler function', typname => 'table_am_handler', typlen => '4', typbyval => 't', typtype => 'p', diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 06dfdfef721..332628d67cc 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -87,6 +87,7 @@ typedef struct CopyFormatOptions CopyLogVerbosityChoice log_verbosity; /* verbosity of logged messages */ int64 reject_limit; /* maximum tolerable number of errors */ List *convert_select; /* list of column names (can be NIL) */ + Oid handler; /* handler function for custom format routine */ } CopyFormatOptions; /* These are private in commands/copy[from|to].c */ diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h index 2a2d2f9876b..4f4ffabf882 100644 --- a/src/include/commands/copyapi.h +++ b/src/include/commands/copyapi.h @@ -22,6 +22,8 @@ */ typedef struct CopyToRoutine { + NodeTag type; + /* * Set output function information. This callback is called once at the * beginning of COPY TO. diff --git a/src/include/nodes/meson.build b/src/include/nodes/meson.build index d1ca24dd32f..96e70e7f38b 100644 --- a/src/include/nodes/meson.build +++ b/src/include/nodes/meson.build @@ -12,6 +12,7 @@ node_support_input_i = [ 'access/sdir.h', 'access/tableam.h', 'access/tsmapi.h', + 'commands/copyapi.h', 'commands/event_trigger.h', 'commands/trigger.h', 'executor/tuptable.h', diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 4e4be3fa511..c9da440eed0 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -16,6 +16,7 @@ SUBDIRS = \ spgist_name_ops \ test_bloomfilter \ test_copy_callbacks \ + test_copy_format \ test_custom_rmgrs \ test_ddl_deparse \ test_dsa \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 2b057451473..d33bbbd4092 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -15,6 +15,7 @@ subdir('spgist_name_ops') subdir('ssl_passphrase_callback') subdir('test_bloomfilter') subdir('test_copy_callbacks') +subdir('test_copy_format') subdir('test_custom_rmgrs') subdir('test_ddl_deparse') subdir('test_dsa') diff --git a/src/test/modules/test_copy_format/.gitignore b/src/test/modules/test_copy_format/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_copy_format/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_copy_format/Makefile b/src/test/modules/test_copy_format/Makefile new file mode 100644 index 00000000000..8497f91624d --- /dev/null +++ b/src/test/modules/test_copy_format/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_copy_format/Makefile + +MODULE_big = test_copy_format +OBJS = \ + $(WIN32RES) \ + test_copy_format.o +PGFILEDESC = "test_copy_format - test custom COPY FORMAT" + +EXTENSION = test_copy_format +DATA = test_copy_format--1.0.sql + +REGRESS = test_copy_format + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_copy_format +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_copy_format/expected/test_copy_format.out b/src/test/modules/test_copy_format/expected/test_copy_format.out new file mode 100644 index 00000000000..adfe7d1572a --- /dev/null +++ b/src/test/modules/test_copy_format/expected/test_copy_format.out @@ -0,0 +1,17 @@ +CREATE EXTENSION test_copy_format; +CREATE TABLE public.test (a smallint, b integer, c bigint); +INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); +ERROR: COPY format "test_copy_format" not recognized +LINE 1: COPY public.test FROM stdin WITH (FORMAT 'test_copy_format')... + ^ +COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); +NOTICE: test_copy_format: is_from=false +NOTICE: CopyToOutFunc: atttypid=21 +NOTICE: CopyToOutFunc: atttypid=23 +NOTICE: CopyToOutFunc: atttypid=20 +NOTICE: CopyToStart: natts=3 +NOTICE: CopyToOneRow: tts_nvalid=3 +NOTICE: CopyToOneRow: tts_nvalid=3 +NOTICE: CopyToOneRow: tts_nvalid=3 +NOTICE: CopyToEnd diff --git a/src/test/modules/test_copy_format/meson.build b/src/test/modules/test_copy_format/meson.build new file mode 100644 index 00000000000..a45a2e0a039 --- /dev/null +++ b/src/test/modules/test_copy_format/meson.build @@ -0,0 +1,33 @@ +# Copyright (c) 2025, PostgreSQL Global Development Group + +test_copy_format_sources = files( + 'test_copy_format.c', +) + +if host_system == 'windows' + test_copy_format_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_copy_format', + '--FILEDESC', 'test_copy_format - test custom COPY FORMAT',]) +endif + +test_copy_format = shared_module('test_copy_format', + test_copy_format_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_copy_format + +test_install_data += files( + 'test_copy_format.control', + 'test_copy_format--1.0.sql', +) + +tests += { + 'name': 'test_copy_format', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_copy_format', + ], + }, +} diff --git a/src/test/modules/test_copy_format/sql/test_copy_format.sql b/src/test/modules/test_copy_format/sql/test_copy_format.sql new file mode 100644 index 00000000000..810b3d8cedc --- /dev/null +++ b/src/test/modules/test_copy_format/sql/test_copy_format.sql @@ -0,0 +1,5 @@ +CREATE EXTENSION test_copy_format; +CREATE TABLE public.test (a smallint, b integer, c bigint); +INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); +COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); diff --git a/src/test/modules/test_copy_format/test_copy_format--1.0.sql b/src/test/modules/test_copy_format/test_copy_format--1.0.sql new file mode 100644 index 00000000000..d24ea03ce99 --- /dev/null +++ b/src/test/modules/test_copy_format/test_copy_format--1.0.sql @@ -0,0 +1,8 @@ +/* src/test/modules/test_copy_format/test_copy_format--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_copy_format" to load this file. \quit + +CREATE FUNCTION test_copy_format(internal) + RETURNS copy_handler + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c new file mode 100644 index 00000000000..b42d472d851 --- /dev/null +++ b/src/test/modules/test_copy_format/test_copy_format.c @@ -0,0 +1,63 @@ +/*-------------------------------------------------------------------------- + * + * test_copy_format.c + * Code for testing custom COPY format. + * + * Portions Copyright (c) 2025, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_copy_format/test_copy_format.c + * + * ------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "commands/copyapi.h" +#include "commands/defrem.h" + +PG_MODULE_MAGIC; + +static void +CopyToOutFunc(CopyToState cstate, Oid atttypid, FmgrInfo *finfo) +{ + ereport(NOTICE, (errmsg("CopyToOutFunc: atttypid=%d", atttypid))); +} + +static void +CopyToStart(CopyToState cstate, TupleDesc tupDesc) +{ + ereport(NOTICE, (errmsg("CopyToStart: natts=%d", tupDesc->natts))); +} + +static void +CopyToOneRow(CopyToState cstate, TupleTableSlot *slot) +{ + ereport(NOTICE, (errmsg("CopyToOneRow: tts_nvalid=%u", slot->tts_nvalid))); +} + +static void +CopyToEnd(CopyToState cstate) +{ + ereport(NOTICE, (errmsg("CopyToEnd"))); +} + +static const CopyToRoutine CopyToRoutineTestCopyFormat = { + .type = T_CopyToRoutine, + .CopyToOutFunc = CopyToOutFunc, + .CopyToStart = CopyToStart, + .CopyToOneRow = CopyToOneRow, + .CopyToEnd = CopyToEnd, +}; + +PG_FUNCTION_INFO_V1(test_copy_format); +Datum +test_copy_format(PG_FUNCTION_ARGS) +{ + bool is_from = PG_GETARG_BOOL(0); + + ereport(NOTICE, + (errmsg("test_copy_format: is_from=%s", is_from ? "true" : "false"))); + + PG_RETURN_POINTER(&CopyToRoutineTestCopyFormat); +} diff --git a/src/test/modules/test_copy_format/test_copy_format.control b/src/test/modules/test_copy_format/test_copy_format.control new file mode 100644 index 00000000000..f05a6362358 --- /dev/null +++ b/src/test/modules/test_copy_format/test_copy_format.control @@ -0,0 +1,4 @@ +comment = 'Test code for custom COPY format' +default_version = '1.0' +module_pathname = '$libdir/test_copy_format' +relocatable = true -- 2.47.2 [text/x-patch] v36-0002-Export-CopyToStateData-as-private-data.patch (9.7K, ../../[email protected]/3-v36-0002-Export-CopyToStateData-as-private-data.patch) download | inline diff: From 8768ecf89603767a3a3f9f96d569e5bc8c8f0c81 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Mon, 25 Nov 2024 13:58:33 +0900 Subject: [PATCH v36 2/7] Export CopyToStateData as private data It's for custom COPY TO format handlers implemented as extension. This just moves codes. This doesn't change codes except CopyDest enum values. CopyDest/CopyFrom enum values such as COPY_FILE are conflicted each other. So COPY_DEST_ prefix instead of COPY_ prefix is used for CopyDest enum values. For example, COPY_FILE in CopyDest is renamed to COPY_DEST_FILE. Note that this isn't enough to implement custom COPY TO format handlers as extension. We'll do the followings in a subsequent commit: 1. Add an opaque space for custom COPY TO format handler 2. Export CopySendEndOfRow() to flush buffer --- src/backend/commands/copyto.c | 78 +++--------------------- src/include/commands/copy.h | 2 +- src/include/commands/copyto_internal.h | 83 ++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 70 deletions(-) create mode 100644 src/include/commands/copyto_internal.h diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index fce8501dc30..99c2f2dd699 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -20,6 +20,7 @@ #include "access/tableam.h" #include "commands/copyapi.h" +#include "commands/copyto_internal.h" #include "commands/progress.h" #include "executor/execdesc.h" #include "executor/executor.h" @@ -36,67 +37,6 @@ #include "utils/rel.h" #include "utils/snapmgr.h" -/* - * Represents the different dest cases we need to worry about at - * the bottom level - */ -typedef enum CopyDest -{ - COPY_FILE, /* to file (or a piped program) */ - COPY_FRONTEND, /* to frontend */ - COPY_CALLBACK, /* to callback function */ -} CopyDest; - -/* - * This struct contains all the state variables used throughout a COPY TO - * operation. - * - * Multi-byte encodings: all supported client-side encodings encode multi-byte - * characters by having the first byte's high bit set. Subsequent bytes of the - * character can have the high bit not set. When scanning data in such an - * encoding to look for a match to a single-byte (ie ASCII) character, we must - * use the full pg_encoding_mblen() machinery to skip over multibyte - * characters, else we might find a false match to a trailing byte. In - * supported server encodings, there is no possibility of a false match, and - * it's faster to make useless comparisons to trailing bytes than it is to - * invoke pg_encoding_mblen() to skip over them. encoding_embeds_ascii is true - * when we have to do it the hard way. - */ -typedef struct CopyToStateData -{ - /* format-specific routines */ - const CopyToRoutine *routine; - - /* low-level state data */ - CopyDest copy_dest; /* type of copy source/destination */ - FILE *copy_file; /* used if copy_dest == COPY_FILE */ - StringInfo fe_msgbuf; /* used for all dests during COPY TO */ - - int file_encoding; /* file or remote side's character encoding */ - bool need_transcoding; /* file encoding diff from server? */ - bool encoding_embeds_ascii; /* ASCII can be non-first byte? */ - - /* parameters from the COPY command */ - Relation rel; /* relation to copy to */ - QueryDesc *queryDesc; /* executable query to copy from */ - List *attnumlist; /* integer list of attnums to copy */ - char *filename; /* filename, or NULL for STDOUT */ - bool is_program; /* is 'filename' a program to popen? */ - copy_data_dest_cb data_dest_cb; /* function for writing data */ - - CopyFormatOptions opts; - Node *whereClause; /* WHERE condition (or NULL) */ - - /* - * Working state - */ - MemoryContext copycontext; /* per-copy execution context */ - - FmgrInfo *out_functions; /* lookup info for output functions */ - MemoryContext rowcontext; /* per-row evaluation context */ - uint64 bytes_processed; /* number of bytes processed so far */ -} CopyToStateData; - /* DestReceiver for COPY (query) TO */ typedef struct { @@ -421,7 +361,7 @@ SendCopyBegin(CopyToState cstate) for (i = 0; i < natts; i++) pq_sendint16(&buf, format); /* per-column formats */ pq_endmessage(&buf); - cstate->copy_dest = COPY_FRONTEND; + cstate->copy_dest = COPY_DEST_FRONTEND; } static void @@ -468,7 +408,7 @@ CopySendEndOfRow(CopyToState cstate) switch (cstate->copy_dest) { - case COPY_FILE: + case COPY_DEST_FILE: if (fwrite(fe_msgbuf->data, fe_msgbuf->len, 1, cstate->copy_file) != 1 || ferror(cstate->copy_file)) @@ -502,11 +442,11 @@ CopySendEndOfRow(CopyToState cstate) errmsg("could not write to COPY file: %m"))); } break; - case COPY_FRONTEND: + case COPY_DEST_FRONTEND: /* Dump the accumulated row as one CopyData message */ (void) pq_putmessage(PqMsg_CopyData, fe_msgbuf->data, fe_msgbuf->len); break; - case COPY_CALLBACK: + case COPY_DEST_CALLBACK: cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len); break; } @@ -527,7 +467,7 @@ CopySendTextLikeEndOfRow(CopyToState cstate) { switch (cstate->copy_dest) { - case COPY_FILE: + case COPY_DEST_FILE: /* Default line termination depends on platform */ #ifndef WIN32 CopySendChar(cstate, '\n'); @@ -535,7 +475,7 @@ CopySendTextLikeEndOfRow(CopyToState cstate) CopySendString(cstate, "\r\n"); #endif break; - case COPY_FRONTEND: + case COPY_DEST_FRONTEND: /* The FE/BE protocol uses \n as newline for all platforms */ CopySendChar(cstate, '\n'); break; @@ -920,12 +860,12 @@ BeginCopyTo(ParseState *pstate, /* See Multibyte encoding comment above */ cstate->encoding_embeds_ascii = PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding); - cstate->copy_dest = COPY_FILE; /* default */ + cstate->copy_dest = COPY_DEST_FILE; /* default */ if (data_dest_cb) { progress_vals[1] = PROGRESS_COPY_TYPE_CALLBACK; - cstate->copy_dest = COPY_CALLBACK; + cstate->copy_dest = COPY_DEST_CALLBACK; cstate->data_dest_cb = data_dest_cb; } else if (pipe) diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 332628d67cc..6df1f8a3b9b 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -90,7 +90,7 @@ typedef struct CopyFormatOptions Oid handler; /* handler function for custom format routine */ } CopyFormatOptions; -/* These are private in commands/copy[from|to].c */ +/* These are private in commands/copy[from|to]_internal.h */ typedef struct CopyFromStateData *CopyFromState; typedef struct CopyToStateData *CopyToState; diff --git a/src/include/commands/copyto_internal.h b/src/include/commands/copyto_internal.h new file mode 100644 index 00000000000..1b58b36c0a3 --- /dev/null +++ b/src/include/commands/copyto_internal.h @@ -0,0 +1,83 @@ +/*------------------------------------------------------------------------- + * + * copyto_internal.h + * Internal definitions for COPY TO command. + * + * + * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/commands/copyto_internal.h + * + *------------------------------------------------------------------------- + */ +#ifndef COPYTO_INTERNAL_H +#define COPYTO_INTERNAL_H + +#include "commands/copy.h" +#include "executor/execdesc.h" +#include "executor/tuptable.h" +#include "nodes/execnodes.h" + +/* + * Represents the different dest cases we need to worry about at + * the bottom level + */ +typedef enum CopyDest +{ + COPY_DEST_FILE, /* to file (or a piped program) */ + COPY_DEST_FRONTEND, /* to frontend */ + COPY_DEST_CALLBACK, /* to callback function */ +} CopyDest; + +/* + * This struct contains all the state variables used throughout a COPY TO + * operation. + * + * Multi-byte encodings: all supported client-side encodings encode multi-byte + * characters by having the first byte's high bit set. Subsequent bytes of the + * character can have the high bit not set. When scanning data in such an + * encoding to look for a match to a single-byte (ie ASCII) character, we must + * use the full pg_encoding_mblen() machinery to skip over multibyte + * characters, else we might find a false match to a trailing byte. In + * supported server encodings, there is no possibility of a false match, and + * it's faster to make useless comparisons to trailing bytes than it is to + * invoke pg_encoding_mblen() to skip over them. encoding_embeds_ascii is true + * when we have to do it the hard way. + */ +typedef struct CopyToStateData +{ + /* format-specific routines */ + const struct CopyToRoutine *routine; + + /* low-level state data */ + CopyDest copy_dest; /* type of copy source/destination */ + FILE *copy_file; /* used if copy_dest == COPY_FILE */ + StringInfo fe_msgbuf; /* used for all dests during COPY TO */ + + int file_encoding; /* file or remote side's character encoding */ + bool need_transcoding; /* file encoding diff from server? */ + bool encoding_embeds_ascii; /* ASCII can be non-first byte? */ + + /* parameters from the COPY command */ + Relation rel; /* relation to copy to */ + QueryDesc *queryDesc; /* executable query to copy from */ + List *attnumlist; /* integer list of attnums to copy */ + char *filename; /* filename, or NULL for STDOUT */ + bool is_program; /* is 'filename' a program to popen? */ + copy_data_dest_cb data_dest_cb; /* function for writing data */ + + CopyFormatOptions opts; + Node *whereClause; /* WHERE condition (or NULL) */ + + /* + * Working state + */ + MemoryContext copycontext; /* per-copy execution context */ + + FmgrInfo *out_functions; /* lookup info for output functions */ + MemoryContext rowcontext; /* per-row evaluation context */ + uint64 bytes_processed; /* number of bytes processed so far */ +} CopyToStateData; + +#endif /* COPYTO_INTERNAL_H */ -- 2.47.2 [text/x-patch] v36-0003-Add-support-for-implementing-custom-COPY-TO-form.patch (2.4K, ../../[email protected]/4-v36-0003-Add-support-for-implementing-custom-COPY-TO-form.patch) download | inline diff: From 427ec09b5cb438e7884d04027acdc207e9d4c80e Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Mon, 25 Nov 2024 14:01:18 +0900 Subject: [PATCH v36 3/7] Add support for implementing custom COPY TO format as extension * Add CopyToStateData::opaque that can be used to keep data for custom COPY TO format implementation * Export CopySendEndOfRow() to flush data in CopyToStateData::fe_msgbuf as CopyToStateFlush() --- src/backend/commands/copyto.c | 12 ++++++++++++ src/include/commands/copyapi.h | 2 ++ src/include/commands/copyto_internal.h | 3 +++ 3 files changed, 17 insertions(+) diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index 99c2f2dd699..f5ed3efbace 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -458,6 +458,18 @@ CopySendEndOfRow(CopyToState cstate) resetStringInfo(fe_msgbuf); } +/* + * Export CopySendEndOfRow() for extensions. We want to keep + * CopySendEndOfRow() as a static function for + * optimization. CopySendEndOfRow() calls in this file may be optimized by a + * compiler. + */ +void +CopyToStateFlush(CopyToState cstate) +{ + CopySendEndOfRow(cstate); +} + /* * Wrapper function of CopySendEndOfRow for text and CSV formats. Sends the * line termination and do common appropriate things for the end of row. diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h index 4f4ffabf882..5c5ea6592e3 100644 --- a/src/include/commands/copyapi.h +++ b/src/include/commands/copyapi.h @@ -56,6 +56,8 @@ typedef struct CopyToRoutine void (*CopyToEnd) (CopyToState cstate); } CopyToRoutine; +extern void CopyToStateFlush(CopyToState cstate); + /* * API structure for a COPY FROM format implementation. Note this must be * allocated in a server-lifetime manner, typically as a static const struct. diff --git a/src/include/commands/copyto_internal.h b/src/include/commands/copyto_internal.h index 1b58b36c0a3..ce1c33a4004 100644 --- a/src/include/commands/copyto_internal.h +++ b/src/include/commands/copyto_internal.h @@ -78,6 +78,9 @@ typedef struct CopyToStateData FmgrInfo *out_functions; /* lookup info for output functions */ MemoryContext rowcontext; /* per-row evaluation context */ uint64 bytes_processed; /* number of bytes processed so far */ + + /* For custom format implementation */ + void *opaque; /* private space */ } CopyToStateData; #endif /* COPYTO_INTERNAL_H */ -- 2.47.2 [text/x-patch] v36-0004-Add-support-for-adding-custom-COPY-FROM-format.patch (8.7K, ../../[email protected]/5-v36-0004-Add-support-for-adding-custom-COPY-FROM-format.patch) download | inline diff: From fb9fb349e1d08a1adaf07658ccc46e8ea1439a80 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Mon, 25 Nov 2024 14:11:55 +0900 Subject: [PATCH v36 4/7] Add support for adding custom COPY FROM format This uses the same handler for COPY TO and COPY FROM but uses different routine. This uses CopyToRoutine for COPY TO and CopyFromRoutine for COPY FROM. PostgreSQL calls a COPY TO/FROM handler with "is_from" argument. It's true for COPY FROM and false for COPY TO: copy_handler(true) returns CopyToRoutine copy_handler(false) returns CopyFromRoutine This also add a test module for custom COPY FROM handler. --- src/backend/commands/copy.c | 13 +++---- src/backend/commands/copyfrom.c | 28 +++++++++++-- src/include/catalog/pg_type.dat | 2 +- src/include/commands/copyapi.h | 2 + .../expected/test_copy_format.out | 10 +++-- .../test_copy_format/sql/test_copy_format.sql | 1 + .../test_copy_format/test_copy_format.c | 39 ++++++++++++++++++- 7 files changed, 78 insertions(+), 17 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 8d94bc313eb..b4417bb6819 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -483,8 +483,8 @@ defGetCopyLogVerbosityChoice(DefElem *def, ParseState *pstate) * This function checks whether the option value is a built-in format such as * "text" and "csv" or not. If the option value isn't a built-in format, this * function finds a COPY format handler that returns a CopyToRoutine (for - * is_from == false). If no COPY format handler is found, this function - * reports an error. + * is_from == false) or CopyFromRountine (for is_from == true). If no COPY + * format handler is found, this function reports an error. */ static void ProcessCopyOptionFormat(ParseState *pstate, @@ -518,12 +518,9 @@ ProcessCopyOptionFormat(ParseState *pstate, } /* custom format */ - if (!is_from) - { - funcargtypes[0] = INTERNALOID; - handlerOid = LookupFuncName(list_make1(makeString(format)), 1, - funcargtypes, true); - } + funcargtypes[0] = INTERNALOID; + handlerOid = LookupFuncName(list_make1(makeString(format)), 1, + funcargtypes, true); if (!OidIsValid(handlerOid)) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index bcf66f0adf8..0809766f910 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -129,6 +129,7 @@ static void CopyFromBinaryEnd(CopyFromState cstate); /* text format */ static const CopyFromRoutine CopyFromRoutineText = { + .type = T_CopyFromRoutine, .CopyFromInFunc = CopyFromTextLikeInFunc, .CopyFromStart = CopyFromTextLikeStart, .CopyFromOneRow = CopyFromTextOneRow, @@ -137,6 +138,7 @@ static const CopyFromRoutine CopyFromRoutineText = { /* CSV format */ static const CopyFromRoutine CopyFromRoutineCSV = { + .type = T_CopyFromRoutine, .CopyFromInFunc = CopyFromTextLikeInFunc, .CopyFromStart = CopyFromTextLikeStart, .CopyFromOneRow = CopyFromCSVOneRow, @@ -145,6 +147,7 @@ static const CopyFromRoutine CopyFromRoutineCSV = { /* binary format */ static const CopyFromRoutine CopyFromRoutineBinary = { + .type = T_CopyFromRoutine, .CopyFromInFunc = CopyFromBinaryInFunc, .CopyFromStart = CopyFromBinaryStart, .CopyFromOneRow = CopyFromBinaryOneRow, @@ -155,13 +158,30 @@ static const CopyFromRoutine CopyFromRoutineBinary = { static const CopyFromRoutine * CopyFromGetRoutine(const CopyFormatOptions *opts) { - if (opts->csv_mode) + if (OidIsValid(opts->handler)) + { + Datum datum; + Node *routine; + + datum = OidFunctionCall1(opts->handler, BoolGetDatum(true)); + routine = (Node *) DatumGetPointer(datum); + if (routine == NULL || !IsA(routine, CopyFromRoutine)) + ereport( + ERROR, + (errcode( + ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY handler function " + "%u did not return " + "CopyFromRoutine struct", + opts->handler))); + return castNode(CopyFromRoutine, routine); + } + else if (opts->csv_mode) return &CopyFromRoutineCSV; else if (opts->binary) return &CopyFromRoutineBinary; - - /* default is text */ - return &CopyFromRoutineText; + else + return &CopyFromRoutineText; } /* Implementation of the start callback for text and CSV formats */ diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat index 340e0cd0a8d..63b7d65f982 100644 --- a/src/include/catalog/pg_type.dat +++ b/src/include/catalog/pg_type.dat @@ -634,7 +634,7 @@ typoutput => 'tsm_handler_out', typreceive => '-', typsend => '-', typalign => 'i' }, { oid => '8752', - descr => 'pseudo-type for the result of a copy to method function', + descr => 'pseudo-type for the result of a copy to/from method function', typname => 'copy_handler', typlen => '4', typbyval => 't', typtype => 'p', typcategory => 'P', typinput => 'copy_handler_in', typoutput => 'copy_handler_out', typreceive => '-', typsend => '-', diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h index 5c5ea6592e3..895c105d8d8 100644 --- a/src/include/commands/copyapi.h +++ b/src/include/commands/copyapi.h @@ -64,6 +64,8 @@ extern void CopyToStateFlush(CopyToState cstate); */ typedef struct CopyFromRoutine { + NodeTag type; + /* * Set input function information. This callback is called once at the * beginning of COPY FROM. diff --git a/src/test/modules/test_copy_format/expected/test_copy_format.out b/src/test/modules/test_copy_format/expected/test_copy_format.out index adfe7d1572a..016893e7026 100644 --- a/src/test/modules/test_copy_format/expected/test_copy_format.out +++ b/src/test/modules/test_copy_format/expected/test_copy_format.out @@ -2,9 +2,13 @@ CREATE EXTENSION test_copy_format; CREATE TABLE public.test (a smallint, b integer, c bigint); INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); -ERROR: COPY format "test_copy_format" not recognized -LINE 1: COPY public.test FROM stdin WITH (FORMAT 'test_copy_format')... - ^ +NOTICE: test_copy_format: is_from=true +NOTICE: CopyFromInFunc: atttypid=21 +NOTICE: CopyFromInFunc: atttypid=23 +NOTICE: CopyFromInFunc: atttypid=20 +NOTICE: CopyFromStart: natts=3 +NOTICE: CopyFromOneRow +NOTICE: CopyFromEnd COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); NOTICE: test_copy_format: is_from=false NOTICE: CopyToOutFunc: atttypid=21 diff --git a/src/test/modules/test_copy_format/sql/test_copy_format.sql b/src/test/modules/test_copy_format/sql/test_copy_format.sql index 810b3d8cedc..0dfdfa00080 100644 --- a/src/test/modules/test_copy_format/sql/test_copy_format.sql +++ b/src/test/modules/test_copy_format/sql/test_copy_format.sql @@ -2,4 +2,5 @@ CREATE EXTENSION test_copy_format; CREATE TABLE public.test (a smallint, b integer, c bigint); INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); +\. COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c index b42d472d851..abafc668463 100644 --- a/src/test/modules/test_copy_format/test_copy_format.c +++ b/src/test/modules/test_copy_format/test_copy_format.c @@ -18,6 +18,40 @@ PG_MODULE_MAGIC; +static void +CopyFromInFunc(CopyFromState cstate, Oid atttypid, + FmgrInfo *finfo, Oid *typioparam) +{ + ereport(NOTICE, (errmsg("CopyFromInFunc: atttypid=%d", atttypid))); +} + +static void +CopyFromStart(CopyFromState cstate, TupleDesc tupDesc) +{ + ereport(NOTICE, (errmsg("CopyFromStart: natts=%d", tupDesc->natts))); +} + +static bool +CopyFromOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls) +{ + ereport(NOTICE, (errmsg("CopyFromOneRow"))); + return false; +} + +static void +CopyFromEnd(CopyFromState cstate) +{ + ereport(NOTICE, (errmsg("CopyFromEnd"))); +} + +static const CopyFromRoutine CopyFromRoutineTestCopyFormat = { + .type = T_CopyFromRoutine, + .CopyFromInFunc = CopyFromInFunc, + .CopyFromStart = CopyFromStart, + .CopyFromOneRow = CopyFromOneRow, + .CopyFromEnd = CopyFromEnd, +}; + static void CopyToOutFunc(CopyToState cstate, Oid atttypid, FmgrInfo *finfo) { @@ -59,5 +93,8 @@ test_copy_format(PG_FUNCTION_ARGS) ereport(NOTICE, (errmsg("test_copy_format: is_from=%s", is_from ? "true" : "false"))); - PG_RETURN_POINTER(&CopyToRoutineTestCopyFormat); + if (is_from) + PG_RETURN_POINTER(&CopyFromRoutineTestCopyFormat); + else + PG_RETURN_POINTER(&CopyToRoutineTestCopyFormat); } -- 2.47.2 [text/x-patch] v36-0005-Use-COPY_SOURCE_-prefix-for-CopySource-enum-valu.patch (3.5K, ../../[email protected]/6-v36-0005-Use-COPY_SOURCE_-prefix-for-CopySource-enum-valu.patch) download | inline diff: From dcf1598a31557af0ade5758430884489a7c8e610 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Mon, 25 Nov 2024 14:19:34 +0900 Subject: [PATCH v36 5/7] Use COPY_SOURCE_ prefix for CopySource enum values This is for consistency with CopyDest. --- src/backend/commands/copyfrom.c | 4 ++-- src/backend/commands/copyfromparse.c | 10 +++++----- src/include/commands/copyfrom_internal.h | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 0809766f910..76662e04260 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -1729,7 +1729,7 @@ BeginCopyFrom(ParseState *pstate, pg_encoding_to_char(GetDatabaseEncoding())))); } - cstate->copy_src = COPY_FILE; /* default */ + cstate->copy_src = COPY_SOURCE_FILE; /* default */ cstate->whereClause = whereClause; @@ -1857,7 +1857,7 @@ BeginCopyFrom(ParseState *pstate, if (data_source_cb) { progress_vals[1] = PROGRESS_COPY_TYPE_CALLBACK; - cstate->copy_src = COPY_CALLBACK; + cstate->copy_src = COPY_SOURCE_CALLBACK; cstate->data_source_cb = data_source_cb; } else if (pipe) diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index e8128f85e6b..17e51f02e04 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -180,7 +180,7 @@ ReceiveCopyBegin(CopyFromState cstate) for (i = 0; i < natts; i++) pq_sendint16(&buf, format); /* per-column formats */ pq_endmessage(&buf); - cstate->copy_src = COPY_FRONTEND; + cstate->copy_src = COPY_SOURCE_FRONTEND; cstate->fe_msgbuf = makeStringInfo(); /* We *must* flush here to ensure FE knows it can send. */ pq_flush(); @@ -248,7 +248,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) switch (cstate->copy_src) { - case COPY_FILE: + case COPY_SOURCE_FILE: bytesread = fread(databuf, 1, maxread, cstate->copy_file); if (ferror(cstate->copy_file)) ereport(ERROR, @@ -257,7 +257,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) if (bytesread == 0) cstate->raw_reached_eof = true; break; - case COPY_FRONTEND: + case COPY_SOURCE_FRONTEND: while (maxread > 0 && bytesread < minread && !cstate->raw_reached_eof) { int avail; @@ -340,7 +340,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) bytesread += avail; } break; - case COPY_CALLBACK: + case COPY_SOURCE_CALLBACK: bytesread = cstate->data_source_cb(databuf, minread, maxread); break; } @@ -1172,7 +1172,7 @@ CopyReadLine(CopyFromState cstate, bool is_csv) * after \. up to the protocol end of copy data. (XXX maybe better * not to treat \. as special?) */ - if (cstate->copy_src == COPY_FRONTEND) + if (cstate->copy_src == COPY_SOURCE_FRONTEND) { int inbytes; diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index c8b22af22d8..3a306e3286e 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -24,9 +24,9 @@ */ typedef enum CopySource { - COPY_FILE, /* from file (or a piped program) */ - COPY_FRONTEND, /* from frontend */ - COPY_CALLBACK, /* from callback function */ + COPY_SOURCE_FILE, /* from file (or a piped program) */ + COPY_SOURCE_FRONTEND, /* from frontend */ + COPY_SOURCE_CALLBACK, /* from callback function */ } CopySource; /* -- 2.47.2 [text/x-patch] v36-0006-Add-support-for-implementing-custom-COPY-FROM-fo.patch (2.4K, ../../[email protected]/7-v36-0006-Add-support-for-implementing-custom-COPY-FROM-fo.patch) download | inline diff: From d8657b0d0d295c82ec2807a9767bb7fbe18c85fa Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Mon, 25 Nov 2024 14:21:39 +0900 Subject: [PATCH v36 6/7] Add support for implementing custom COPY FROM format as extension * Add CopyFromStateData::opaque that can be used to keep data for custom COPY From format implementation * Export CopyGetData() to get the next data as CopyFromStateGetData() --- src/backend/commands/copyfromparse.c | 11 +++++++++++ src/include/commands/copyapi.h | 2 ++ src/include/commands/copyfrom_internal.h | 3 +++ 3 files changed, 16 insertions(+) diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 17e51f02e04..d8fd238e72b 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -739,6 +739,17 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes) return copied_bytes; } +/* + * Export CopyGetData() for extensions. We want to keep CopyGetData() as a + * static function for optimization. CopyGetData() calls in this file may be + * optimized by a compiler. + */ +int +CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread) +{ + return CopyGetData(cstate, dest, minread, maxread); +} + /* * This function is exposed for use by extensions that read raw fields in the * next line. See NextCopyFromRawFieldsInternal() for details. diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h index 895c105d8d8..2044d8b8c4c 100644 --- a/src/include/commands/copyapi.h +++ b/src/include/commands/copyapi.h @@ -108,4 +108,6 @@ typedef struct CopyFromRoutine void (*CopyFromEnd) (CopyFromState cstate); } CopyFromRoutine; +extern int CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread); + #endif /* COPYAPI_H */ diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h index 3a306e3286e..af425cf5fd9 100644 --- a/src/include/commands/copyfrom_internal.h +++ b/src/include/commands/copyfrom_internal.h @@ -181,6 +181,9 @@ typedef struct CopyFromStateData #define RAW_BUF_BYTES(cstate) ((cstate)->raw_buf_len - (cstate)->raw_buf_index) uint64 bytes_processed; /* number of bytes processed so far */ + + /* For custom format implementation */ + void *opaque; /* private space */ } CopyFromStateData; extern void ReceiveCopyBegin(CopyFromState cstate); -- 2.47.2 [text/x-patch] v36-0007-Add-CopyFromSkipErrorRow-for-custom-COPY-format-.patch (11.0K, ../../[email protected]/8-v36-0007-Add-CopyFromSkipErrorRow-for-custom-COPY-format-.patch) download | inline diff: From ddd4b9f09dfe7860195adf82dbaab5ee46e91cf0 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Wed, 27 Nov 2024 16:23:55 +0900 Subject: [PATCH v36 7/7] Add CopyFromSkipErrorRow() for custom COPY format extension Extensions must call CopyFromSkipErrorRow() when CopyFromOneRow callback reports an error by errsave(). CopyFromSkipErrorRow() handles "ON_ERROR stop" and "LOG_VERBOSITY verbose" cases. --- src/backend/commands/copyfromparse.c | 82 +++++++++++-------- src/include/commands/copyapi.h | 2 + .../expected/test_copy_format.out | 47 +++++++++++ .../test_copy_format/sql/test_copy_format.sql | 24 ++++++ .../test_copy_format/test_copy_format.c | 80 +++++++++++++++++- 5 files changed, 198 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index d8fd238e72b..2070f51a963 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -938,6 +938,51 @@ CopyFromCSVOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, return CopyFromTextLikeOneRow(cstate, econtext, values, nulls, true); } +/* + * Call this when you report an error by errsave() in your CopyFromOneRow + * callback. This handles "ON_ERROR stop" and "LOG_VERBOSITY verbose" cases + * for you. + */ +void +CopyFromSkipErrorRow(CopyFromState cstate) +{ + Assert(cstate->opts.on_error != COPY_ON_ERROR_STOP); + + cstate->num_errors++; + + if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE) + { + /* + * Since we emit line number and column info in the below notice + * message, we suppress error context information other than the + * relation name. + */ + Assert(!cstate->relname_only); + cstate->relname_only = true; + + if (cstate->cur_attval) + { + char *attval; + + attval = CopyLimitPrintoutLength(cstate->cur_attval); + ereport(NOTICE, + errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": \"%s\"", + (unsigned long long) cstate->cur_lineno, + cstate->cur_attname, + attval)); + pfree(attval); + } + else + ereport(NOTICE, + errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": null input", + (unsigned long long) cstate->cur_lineno, + cstate->cur_attname)); + + /* reset relname_only */ + cstate->relname_only = false; + } +} + /* * Workhorse for CopyFromTextOneRow() and CopyFromCSVOneRow(). * @@ -1044,42 +1089,7 @@ CopyFromTextLikeOneRow(CopyFromState cstate, ExprContext *econtext, (Node *) cstate->escontext, &values[m])) { - Assert(cstate->opts.on_error != COPY_ON_ERROR_STOP); - - cstate->num_errors++; - - if (cstate->opts.log_verbosity == COPY_LOG_VERBOSITY_VERBOSE) - { - /* - * Since we emit line number and column info in the below - * notice message, we suppress error context information other - * than the relation name. - */ - Assert(!cstate->relname_only); - cstate->relname_only = true; - - if (cstate->cur_attval) - { - char *attval; - - attval = CopyLimitPrintoutLength(cstate->cur_attval); - ereport(NOTICE, - errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": \"%s\"", - (unsigned long long) cstate->cur_lineno, - cstate->cur_attname, - attval)); - pfree(attval); - } - else - ereport(NOTICE, - errmsg("skipping row due to data type incompatibility at line %llu for column \"%s\": null input", - (unsigned long long) cstate->cur_lineno, - cstate->cur_attname)); - - /* reset relname_only */ - cstate->relname_only = false; - } - + CopyFromSkipErrorRow(cstate); return true; } diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h index 2044d8b8c4c..500ece7d5bb 100644 --- a/src/include/commands/copyapi.h +++ b/src/include/commands/copyapi.h @@ -110,4 +110,6 @@ typedef struct CopyFromRoutine extern int CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread); +extern void CopyFromSkipErrorRow(CopyFromState cstate); + #endif /* COPYAPI_H */ diff --git a/src/test/modules/test_copy_format/expected/test_copy_format.out b/src/test/modules/test_copy_format/expected/test_copy_format.out index 016893e7026..b9a6baa85c0 100644 --- a/src/test/modules/test_copy_format/expected/test_copy_format.out +++ b/src/test/modules/test_copy_format/expected/test_copy_format.out @@ -1,6 +1,8 @@ CREATE EXTENSION test_copy_format; CREATE TABLE public.test (a smallint, b integer, c bigint); INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); +-- 987 is accepted. +-- 654 is a hard error because ON_ERROR is stop by default. COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); NOTICE: test_copy_format: is_from=true NOTICE: CopyFromInFunc: atttypid=21 @@ -8,7 +10,50 @@ NOTICE: CopyFromInFunc: atttypid=23 NOTICE: CopyFromInFunc: atttypid=20 NOTICE: CopyFromStart: natts=3 NOTICE: CopyFromOneRow +NOTICE: CopyFromOneRow +ERROR: invalid value: "6" +CONTEXT: COPY test, line 2, column a: "6" +-- 987 is accepted. +-- 654 is a soft error because ON_ERROR is ignore. +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore); +NOTICE: test_copy_format: is_from=true +NOTICE: CopyFromInFunc: atttypid=21 +NOTICE: CopyFromInFunc: atttypid=23 +NOTICE: CopyFromInFunc: atttypid=20 +NOTICE: CopyFromStart: natts=3 +NOTICE: CopyFromOneRow +NOTICE: CopyFromOneRow +NOTICE: CopyFromOneRow +NOTICE: 1 row was skipped due to data type incompatibility NOTICE: CopyFromEnd +-- 987 is accepted. +-- 654 is a soft error because ON_ERROR is ignore. +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore, LOG_VERBOSITY verbose); +NOTICE: test_copy_format: is_from=true +NOTICE: CopyFromInFunc: atttypid=21 +NOTICE: CopyFromInFunc: atttypid=23 +NOTICE: CopyFromInFunc: atttypid=20 +NOTICE: CopyFromStart: natts=3 +NOTICE: CopyFromOneRow +NOTICE: CopyFromOneRow +NOTICE: skipping row due to data type incompatibility at line 2 for column "a": "6" +NOTICE: CopyFromOneRow +NOTICE: 1 row was skipped due to data type incompatibility +NOTICE: CopyFromEnd +-- 987 is accepted. +-- 654 is a soft error because ON_ERROR is ignore. +-- 321 is a hard error. +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore); +NOTICE: test_copy_format: is_from=true +NOTICE: CopyFromInFunc: atttypid=21 +NOTICE: CopyFromInFunc: atttypid=23 +NOTICE: CopyFromInFunc: atttypid=20 +NOTICE: CopyFromStart: natts=3 +NOTICE: CopyFromOneRow +NOTICE: CopyFromOneRow +NOTICE: CopyFromOneRow +ERROR: too much lines: 3 +CONTEXT: COPY test, line 3 COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); NOTICE: test_copy_format: is_from=false NOTICE: CopyToOutFunc: atttypid=21 @@ -18,4 +63,6 @@ NOTICE: CopyToStart: natts=3 NOTICE: CopyToOneRow: tts_nvalid=3 NOTICE: CopyToOneRow: tts_nvalid=3 NOTICE: CopyToOneRow: tts_nvalid=3 +NOTICE: CopyToOneRow: tts_nvalid=3 +NOTICE: CopyToOneRow: tts_nvalid=3 NOTICE: CopyToEnd diff --git a/src/test/modules/test_copy_format/sql/test_copy_format.sql b/src/test/modules/test_copy_format/sql/test_copy_format.sql index 0dfdfa00080..86db71bce7f 100644 --- a/src/test/modules/test_copy_format/sql/test_copy_format.sql +++ b/src/test/modules/test_copy_format/sql/test_copy_format.sql @@ -1,6 +1,30 @@ CREATE EXTENSION test_copy_format; CREATE TABLE public.test (a smallint, b integer, c bigint); INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); +-- 987 is accepted. +-- 654 is a hard error because ON_ERROR is stop by default. COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); +987 +654 +\. +-- 987 is accepted. +-- 654 is a soft error because ON_ERROR is ignore. +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore); +987 +654 +\. +-- 987 is accepted. +-- 654 is a soft error because ON_ERROR is ignore. +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore, LOG_VERBOSITY verbose); +987 +654 +\. +-- 987 is accepted. +-- 654 is a soft error because ON_ERROR is ignore. +-- 321 is a hard error. +COPY public.test FROM stdin WITH (FORMAT 'test_copy_format', ON_ERROR ignore); +987 +654 +321 \. COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c index abafc668463..96a54dab7ec 100644 --- a/src/test/modules/test_copy_format/test_copy_format.c +++ b/src/test/modules/test_copy_format/test_copy_format.c @@ -14,6 +14,7 @@ #include "postgres.h" #include "commands/copyapi.h" +#include "commands/copyfrom_internal.h" #include "commands/defrem.h" PG_MODULE_MAGIC; @@ -34,8 +35,85 @@ CopyFromStart(CopyFromState cstate, TupleDesc tupDesc) static bool CopyFromOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls) { + int n_attributes = list_length(cstate->attnumlist); + char *line; + int line_size = n_attributes + 1; /* +1 is for new line */ + int read_bytes; + ereport(NOTICE, (errmsg("CopyFromOneRow"))); - return false; + + cstate->cur_lineno++; + line = palloc(line_size); + read_bytes = CopyFromStateGetData(cstate, line, line_size, line_size); + if (read_bytes == 0) + return false; + if (read_bytes != line_size) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("one line must be %d bytes: %d", + line_size, read_bytes))); + + if (cstate->cur_lineno == 1) + { + /* Success */ + TupleDesc tupDesc = RelationGetDescr(cstate->rel); + ListCell *cur; + int i = 0; + + foreach(cur, cstate->attnumlist) + { + int attnum = lfirst_int(cur); + int m = attnum - 1; + Form_pg_attribute att = TupleDescAttr(tupDesc, m); + + if (att->atttypid == INT2OID) + { + values[i] = Int16GetDatum(line[i] - '0'); + } + else if (att->atttypid == INT4OID) + { + values[i] = Int32GetDatum(line[i] - '0'); + } + else if (att->atttypid == INT8OID) + { + values[i] = Int64GetDatum(line[i] - '0'); + } + nulls[i] = false; + i++; + } + } + else if (cstate->cur_lineno == 2) + { + /* Soft error */ + TupleDesc tupDesc = RelationGetDescr(cstate->rel); + int attnum = lfirst_int(list_head(cstate->attnumlist)); + int m = attnum - 1; + Form_pg_attribute att = TupleDescAttr(tupDesc, m); + char value[2]; + + cstate->cur_attname = NameStr(att->attname); + value[0] = line[0]; + value[1] = '\0'; + cstate->cur_attval = value; + errsave((Node *) cstate->escontext, + ( + errcode(ERRCODE_INVALID_TEXT_REPRESENTATION), + errmsg("invalid value: \"%c\"", line[0]))); + CopyFromSkipErrorRow(cstate); + cstate->cur_attname = NULL; + cstate->cur_attval = NULL; + return true; + } + else + { + /* Hard error */ + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("too much lines: %llu", + (unsigned long long) cstate->cur_lineno))); + } + + return true; } static void -- 2.47.2 ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-17 20:50 ` Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Masahiko Sawada @ 2025-03-17 20:50 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers On Tue, Mar 4, 2025 at 4:06 PM Sutou Kouhei <[email protected]> wrote: > > Hi, > > In <CAD21AoAwOP7p6LgmkPGqPuJ5KbJPPQsSZsFzwCDguwzr9F677Q@mail.gmail.com> > "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 3 Mar 2025 11:06:39 -0800, > Masahiko Sawada <[email protected]> wrote: > > > I agree with the fix and the patch looks good to me. I've updated the > > commit message and am going to push, barring any objections. > > Thanks! > > I've rebased the patch set. Here is a summary again: Thank you for updating the patches. Here are some review comments on the 0001 patch: + if (strcmp(format, "text") == 0) + { + /* "csv_mode == false && binary == false" means "text" */ + return; + } + else if (strcmp(format, "csv") == 0) + { + opts_out->csv_mode = true; + return; + } + else if (strcmp(format, "binary") == 0) + { + opts_out->binary = true; + return; + } + + /* custom format */ + if (!is_from) + { + funcargtypes[0] = INTERNALOID; + handlerOid = LookupFuncName(list_make1(makeString(format)), 1, + funcargtypes, true); + } + if (!OidIsValid(handlerOid)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY format \"%s\" not recognized", format), + parser_errposition(pstate, defel->location))); I think that built-in formats also need to have their handler functions. This seems to be a conventional way for customizable features such as tablesample and access methods, and we can simplify this function. --- I think we need to update the documentation to describe how users can define the handler functions and what each callback function is responsible for. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-19 02:56 ` Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-22 00:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 0 siblings, 2 replies; 24+ messages in thread From: Sutou Kouhei @ 2025-03-19 02:56 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAD21AoDU=bYRDDY8MzCXAfg4h9XTeTBdM-wVJaO1t4UcseCpuA@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 17 Mar 2025 13:50:03 -0700, Masahiko Sawada <[email protected]> wrote: > I think that built-in formats also need to have their handler > functions. This seems to be a conventional way for customizable > features such as tablesample and access methods, and we can simplify > this function. OK. 0008 in the attached v37 patch set does it. > I think we need to update the documentation to describe how users can > define the handler functions and what each callback function is > responsible for. I agree with it but we haven't finalized public APIs yet. Can we defer it after we finalize public APIs? (Proposed public APIs exist in 0003, 0006 and 0007.) And could someone help (take over if possible) writing a document for this feature? I'm not good at writing a document in English... 0009 in the attached v37 patch set has a draft of it. It's based on existing documents in doc/src/sgml/ and *.h. 0001-0007 aren't changed from v36 patch set. Thanks, -- kou ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-20 00:49 ` David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: David G. Johnston @ 2025-03-20 00:49 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Tue, Mar 18, 2025 at 7:56 PM Sutou Kouhei <[email protected]> wrote: > And could someone help (take over if possible) writing a > document for this feature? I'm not good at writing a > document in English... 0009 in the attached v37 patch set > has a draft of it. It's based on existing documents in > doc/src/sgml/ and *.h. > > I haven't touched the innards of the structs aside from changing programlisting to synopsis. And redoing the two section opening paragraphs to better integrate with the content in the chapter opening. The rest I kinda went to town on... David J. diff --git a/doc/src/sgml/copy-handler.sgml b/doc/src/sgml/copy-handler.sgml index f602debae6..9d2897a104 100644 --- a/doc/src/sgml/copy-handler.sgml +++ b/doc/src/sgml/copy-handler.sgml @@ -10,56 +10,72 @@ <para> <productname>PostgreSQL</productname> supports custom <link linkend="sql-copy"><literal>COPY</literal></link> - handlers. The <literal>COPY</literal> handlers can use different copy format - instead of built-in <literal>text</literal>, <literal>csv</literal> - and <literal>binary</literal>. + handlers; adding additional <replaceable>format_name</replaceable> options + to the <literal>FORMAT</literal> clause. </para> <para> - At the SQL level, a table sampling method is represented by a single SQL - function, typically implemented in C, having the signature -<programlisting> -format_name(internal) RETURNS copy_handler -</programlisting> - The name of the function is the same name appearing in - the <literal>FORMAT</literal> option. The <type>internal</type> argument is - a dummy that simply serves to prevent this function from being called - directly from an SQL command. The real argument is <literal>bool - is_from</literal>. If the handler is used by <literal>COPY FROM</literal>, - it's <literal>true</literal>. If the handler is used by <literal>COPY - FROM</literal>, it's <literal>false</literal>. + At the SQL level, a copy handler method is represented by a single SQL + function (see <xref linkend="sql-createfunction"/>), typically implemented in + C, having the signature +<synopsis> +<replaceable>format_name</replaceable>(internal) RETURNS <literal>copy_handler</literal> +</synopsis> + The function's name is then accepted as a valid <replaceable>format_name</replaceable>. + The return pseudo-type <literal>copy_handler</literal> informs the system that + this function needs to be registered as a copy handler. + The <type>internal</type> argument is a dummy that prevents + this function from being called directly from an SQL command. As the + handler implementation must be server-lifetime immutable; this SQL function's + volatility should be marked immutable. The <literal>link_symbol</literal> + for this function is the name of the implementation function, described next. </para> <para> - The function must return <type>CopyFromRoutine *</type> when - the <literal>is_from</literal> argument is <literal>true</literal>. - The function must return <type>CopyToRoutine *</type> when - the <literal>is_from</literal> argument is <literal>false</literal>. + The implementation function signature expected for the function named + in the <literal>link_symbol</literal> is: +<synopsis> +Datum +<replaceable>copy_format_handler</replaceable>(PG_FUNCTION_ARGS) +</synopsis> + The convention for the name is to replace the word + <replaceable>format</replaceable> in the placeholder above with the value given + to <replaceable>format_name</replaceable> in the SQL function. + The first argument is a <type>boolean</type> that indicates whether the handler + must provide a pointer to its implementation for <literal>COPY FROM</literal> + (a <type>CopyFromRoutine *</type>). If <literal>false</literal>, the handler + must provide a pointer to its implementation of <literal>COPY TO</literal> + (a <type>CopyToRoutine *</type>). These structs are declared in + <filename>src/include/commands/copyapi.h</filename>. </para> <para> - The <type>CopyFromRoutine</type> and <type>CopyToRoutine</type> struct types - are declared in <filename>src/include/commands/copyapi.h</filename>, - which see for additional details. + The structs hold pointers to implementation functions for + initializing, starting, processing rows, and ending a copy operation. + The specific structures vary a bit between <literal>COPY FROM</literal> and + <literal>COPY TO</literal> so the next two sections describes each + in detail. </para> <sect1 id="copy-handler-from"> <title>Copy From Handler</title> <para> - The <literal>COPY</literal> handler function for <literal>COPY - FROM</literal> returns a <type>CopyFromRoutine</type> struct containing - pointers to the functions described below. All functions are required. + The opening to this chapter describes how the executor will call the + main handler function with, in this case, + a <type>boolean</type> <literal>true</literal>, and expect to receive a + <type>CopyFromRoutine *</type> <type>Datum</type>. This section describes + the components of the <type>CopyFromRoutine</type> struct. </para> <para> -<programlisting> +<synopsis> void CopyFromInFunc(CopyFromState cstate, Oid atttypid, FmgrInfo *finfo, Oid *typioparam); -</programlisting> +</synopsis> This sets input function information for the given <literal>atttypid</literal> attribute. This function is called once @@ -110,11 +126,11 @@ CopyFromInFunc(CopyFromState cstate, </para> <para> -<programlisting> +<synopsis> void CopyFromStart(CopyFromState cstate, TupleDesc tupDesc); -</programlisting> +</synopsis> This starts a <literal>COPY FROM</literal>. This function is called once at the beginning of <literal>COPY FROM</literal>. @@ -144,13 +160,13 @@ CopyFromStart(CopyFromState cstate, </para> <para> -<programlisting> +<synopsis> bool CopyFromOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls); -</programlisting> +</synopsis> This reads one row from the source and fill <literal>values</literal> and <literal>nulls</literal>. If there is one or more tuples to be read, @@ -202,10 +218,10 @@ CopyFromOneRow(CopyFromState cstate, </para> <para> -<programlisting> +<synopsis> void CopyFromEnd(CopyFromState cstate); -</programlisting> +</synopsis> This ends a <literal>COPY FROM</literal>. This function is called once at the end of <literal>COPY FROM</literal>. @@ -232,18 +248,20 @@ CopyFromEnd(CopyFromState cstate); <title>Copy To Handler</title> <para> - The <literal>COPY</literal> handler function for <literal>COPY - TO</literal> returns a <type>CopyToRoutine</type> struct containing - pointers to the functions described below. All functions are required. + The opening to this chapter describes how the executor will call the + main handler function with, in this case, + a <type>boolean</type> <literal>false</literal>, and expect to receive a + <type>CopyInRoutine *</type> <type>Datum</type>. This section describes + the components of the <type>CopyInRoutine</type> struct. </para> <para> -<programlisting> +<synopsis> void CopyToOutFunc(CopyToState cstate, Oid atttypid, FmgrInfo *finfo); -</programlisting> +</synopsis> This sets output function information for the given <literal>atttypid</literal> attribute. This function is called once @@ -284,11 +302,11 @@ CopyToOutFunc(CopyToState cstate, </para> <para> -<programlisting> +<synopsis> void CopyToStart(CopyToState cstate, TupleDesc tupDesc); -</programlisting> +</synopsis> This starts a <literal>COPY TO</literal>. This function is called once at the beginning of <literal>COPY TO</literal>. @@ -316,11 +334,11 @@ CopyToStart(CopyToState cstate, </para> <para> -<programlisting> +<synopsis> bool CopyToOneRow(CopyToState cstate, TupleTableSlot *slot); -</programlisting> +</synopsis> This writes one row stored in <literal>slot</literal> to the destination. @@ -347,10 +365,10 @@ CopyToOneRow(CopyToState cstate, </para> <para> -<programlisting> +<synopsis> void CopyToEnd(CopyToState cstate); -</programlisting> +</synopsis> This ends a <literal>COPY TO</literal>. This function is called once at the end of <literal>COPY TO</literal>. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> @ 2025-03-20 01:24 ` Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-25 00:45 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 24+ messages in thread From: Sutou Kouhei @ 2025-03-20 01:24 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAKFQuwaMAFMHqxDXR=SxA0mDjdmntrwxZd2w=nSruLNFH-OzLw@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 19 Mar 2025 17:49:49 -0700, "David G. Johnston" <[email protected]> wrote: >> And could someone help (take over if possible) writing a >> document for this feature? I'm not good at writing a >> document in English... 0009 in the attached v37 patch set >> has a draft of it. It's based on existing documents in >> doc/src/sgml/ and *.h. >> >> > I haven't touched the innards of the structs aside from changing > programlisting to synopsis. And redoing the two section opening paragraphs > to better integrate with the content in the chapter opening. > > The rest I kinda went to town on... Thanks!!! It's very helpful!!! I've applied your patch. 0009 is only changed. Thanks, -- kou ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-23 09:01 ` Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Masahiko Sawada @ 2025-03-23 09:01 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Mar 19, 2025 at 6:25 PM Sutou Kouhei <[email protected]> wrote: > > Hi, > > In <CAKFQuwaMAFMHqxDXR=SxA0mDjdmntrwxZd2w=nSruLNFH-OzLw@mail.gmail.com> > "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 19 Mar 2025 17:49:49 -0700, > "David G. Johnston" <[email protected]> wrote: > > >> And could someone help (take over if possible) writing a > >> document for this feature? I'm not good at writing a > >> document in English... 0009 in the attached v37 patch set > >> has a draft of it. It's based on existing documents in > >> doc/src/sgml/ and *.h. > >> > >> > > I haven't touched the innards of the structs aside from changing > > programlisting to synopsis. And redoing the two section opening paragraphs > > to better integrate with the content in the chapter opening. > > > > The rest I kinda went to town on... > > Thanks!!! It's very helpful!!! > > I've applied your patch. 0009 is only changed. Thank you for updating the patches. I've reviewed the main part of supporting the custom COPY format. Here are some random comments: --- +/* + * Process the "format" option. + * + * This function checks whether the option value is a built-in format such as + * "text" and "csv" or not. If the option value isn't a built-in format, this + * function finds a COPY format handler that returns a CopyToRoutine (for + * is_from == false) or CopyFromRountine (for is_from == true). If no COPY + * format handler is found, this function reports an error. + */ I think this comment needs to be updated as the part "If the option value isn't ..." is no longer true. I think we don't necessarily need to create a separate function ProcessCopyOptionFormat for processing the format option. We need more regression tests for handling the given format name. For example, - more various input patterns. - a function with the specified format name exists but it returns an unexpected Node. - looking for a handler function in a different namespace. etc. --- I think that we should accept qualified names too as the format name like tablesample does. That way, different extensions implementing the same format can be used. --- + if (routine == NULL || !IsA(routine, CopyFromRoutine)) + ereport( + ERROR, + (errcode( + ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY handler function " + "%u did not return " + "CopyFromRoutine struct", + opts->handler))); It's not conventional to put a new line between 'ereport(' and 'ERROR' (similarly between 'errcode(' and 'ERRCODE_...'. Also, we don't need to split the error message into multiple lines as it's not long. --- + if (routine == NULL || !IsA(routine, CopyToRoutine)) + ereport( + ERROR, + (errcode( + ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("COPY handler function " + "%u did not return " + "CopyToRoutine struct", + opts->handler))); Same as the above comment. --- + descr => 'pseudo-type for the result of a copy to/from method function', s/method function/format function/ --- + Oid handler; /* handler function for custom format routine */ 'handler' is used also for built-in formats. --- +static void +CopyFromInFunc(CopyFromState cstate, Oid atttypid, + FmgrInfo *finfo, Oid *typioparam) +{ + ereport(NOTICE, (errmsg("CopyFromInFunc: atttypid=%d", atttypid))); +} OIDs could be changed across major versions even for built-in types. I think it's better to avoid using it for tests. --- +static void +CopyToOneRow(CopyToState cstate, TupleTableSlot *slot) +{ + ereport(NOTICE, (errmsg("CopyToOneRow: tts_nvalid=%u", slot->tts_nvalid))); +} Similar to the above comment, the field name 'tts_nvalid' might also be changed in the future, let's use another name. --- +static const CopyFromRoutine CopyFromRoutineTestCopyFormat = { + .type = T_CopyFromRoutine, + .CopyFromInFunc = CopyFromInFunc, + .CopyFromStart = CopyFromStart, + .CopyFromOneRow = CopyFromOneRow, + .CopyFromEnd = CopyFromEnd, +}; I'd suggest not using the same function names as the fields. --- +/* + * Export CopySendEndOfRow() for extensions. We want to keep + * CopySendEndOfRow() as a static function for + * optimization. CopySendEndOfRow() calls in this file may be optimized by a + * compiler. + */ +void +CopyToStateFlush(CopyToState cstate) +{ + CopySendEndOfRow(cstate); +} Is there any reason to use a different name for public functions? --- +/* + * Export CopyGetData() for extensions. We want to keep CopyGetData() as a + * static function for optimization. CopyGetData() calls in this file may be + * optimized by a compiler. + */ +int +CopyFromStateGetData(CopyFromState cstate, void *dest, int minread, int maxread) +{ + return CopyGetData(cstate, dest, minread, maxread); +} + The same as the above comment. --- + /* For custom format implementation */ + void *opaque; /* private space */ How about renaming 'private'? --- I've not reviewed the documentation patch yet but I think the patch seems to miss the updates to the description of the FORMAT option in the COPY command section. --- I think we can reorganize the patch set as follows: 1. Create copyto_internal.h and change COPY_XXX to COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. 2. Support custom format for both COPY TO and COPY FROM. 3. Expose necessary helper functions such as CopySendEndOfRow(). 4. Add CopyFromSkipErrorRow(). 5. Documentation. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-27 03:28 ` Sutou Kouhei <[email protected]> 2025-03-29 05:37 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-29 16:48 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-31 17:05 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 0 siblings, 3 replies; 24+ messages in thread From: Sutou Kouhei @ 2025-03-27 03:28 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAD21AoAfWrjpTDJ0garVUoXY0WC3Ud4Cu51q+ccWiotm1uo_2A@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 23 Mar 2025 02:01:59 -0700, Masahiko Sawada <[email protected]> wrote: > --- > +/* > + * Process the "format" option. > + * > + * This function checks whether the option value is a built-in format such as > + * "text" and "csv" or not. If the option value isn't a built-in format, this > + * function finds a COPY format handler that returns a CopyToRoutine (for > + * is_from == false) or CopyFromRountine (for is_from == true). If no COPY > + * format handler is found, this function reports an error. > + */ > > I think this comment needs to be updated as the part "If the option > value isn't ..." is no longer true. > > I think we don't necessarily need to create a separate function > ProcessCopyOptionFormat for processing the format option. Hmm. I think that this separated function will increase readability by reducing indentation. But I've removed the separation as you suggested. So the comment is also removed entirely. 0002 includes this. > We need more regression tests for handling the given format name. For example, > > - more various input patterns. > - a function with the specified format name exists but it returns an > unexpected Node. > - looking for a handler function in a different namespace. > etc. I've added the following tests: * Wrong input type handler without namespace * Wrong input type handler with namespace * Wrong return type handler without namespace * Wrong return type handler with namespace * Wrong return value (Copy*Routine isn't returned) handler without namespace * Wrong return value (Copy*Routine isn't returned) handler with namespace * Nonexistent handler * Invalid qualified name * Valid handler without namespace and without search_path * Valid handler without namespace and with search_path * Valid handler with namespace 0002 also includes this. > I think that we should accept qualified names too as the format name > like tablesample does. That way, different extensions implementing the > same format can be used. Implemented. It's implemented after parsing SQL. Is it OK? (It seems that tablesample does it in parsing SQL.) Because "WITH (FORMAT XXX)" is processed as a generic option in gram.y. All generic options are processed as strings. So I keep this. Syntax is "COPY ... WITH (FORMAT 'NAMESPACE.HANDLER_NAME')" not "COPY ... WITH (FORMAT 'NAMESPACE'.'HANDLER_NAME')" because of this choice. 0002 also includes this. > --- > + if (routine == NULL || !IsA(routine, CopyFromRoutine)) > + ereport( > + ERROR, > + (errcode( > + > ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("COPY handler function " > + "%u did not return " > + "CopyFromRoutine struct", > + opts->handler))); > > It's not conventional to put a new line between 'ereport(' and 'ERROR' > (similarly between 'errcode(' and 'ERRCODE_...'. Also, we don't need > to split the error message into multiple lines as it's not long. Oh, sorry. I can't remember why I used this... I think I trusted pgindent... > --- > + descr => 'pseudo-type for the result of a copy to/from method function', > > s/method function/format function/ Good catch. I used "handler function" not "format function" because we use "handler" in other places. > --- > + Oid handler; /* handler > function for custom format routine */ > > 'handler' is used also for built-in formats. Updated in 0004. > --- > +static void > +CopyFromInFunc(CopyFromState cstate, Oid atttypid, > + FmgrInfo *finfo, Oid *typioparam) > +{ > + ereport(NOTICE, (errmsg("CopyFromInFunc: atttypid=%d", atttypid))); > +} > > OIDs could be changed across major versions even for built-in types. I > think it's better to avoid using it for tests. Oh, I didn't know it. I've changed to use type name instead of OID. It'll be more stable than OID. > --- > +static void > +CopyToOneRow(CopyToState cstate, TupleTableSlot *slot) > +{ > + ereport(NOTICE, (errmsg("CopyToOneRow: tts_nvalid=%u", > slot->tts_nvalid))); > +} > > Similar to the above comment, the field name 'tts_nvalid' might also > be changed in the future, let's use another name. Hmm. If the field name is changed, we need to change this code. So changing tests too isn't strange. Anyway, I used more generic text. > --- > +static const CopyFromRoutine CopyFromRoutineTestCopyFormat = { > + .type = T_CopyFromRoutine, > + .CopyFromInFunc = CopyFromInFunc, > + .CopyFromStart = CopyFromStart, > + .CopyFromOneRow = CopyFromOneRow, > + .CopyFromEnd = CopyFromEnd, > +}; > > I'd suggest not using the same function names as the fields. OK. I've added "Test" prefix. > --- > +/* > + * Export CopySendEndOfRow() for extensions. We want to keep > + * CopySendEndOfRow() as a static function for > + * optimization. CopySendEndOfRow() calls in this file may be optimized by a > + * compiler. > + */ > +void > +CopyToStateFlush(CopyToState cstate) > +{ > + CopySendEndOfRow(cstate); > +} > > Is there any reason to use a different name for public functions? In this patch set, I use "CopyFrom"/"CopyTo" prefixes for public APIs for custom COPY FORMAT handler extensions. It will help understanding related APIs. Is it strange in PostgreSQL? > --- > + /* For custom format implementation */ > + void *opaque; /* private space */ > > How about renaming 'private'? We should not use "private" because it's a keyword in C++. If we use "private" here, we can't include this file from C++ code. > --- > I've not reviewed the documentation patch yet but I think the patch > seems to miss the updates to the description of the FORMAT option in > the COPY command section. I defer this for now. We can revisit the last documentation patch after we finalize our API. (Or could someone help us?) > I think we can reorganize the patch set as follows: > > 1. Create copyto_internal.h and change COPY_XXX to COPY_SOURCE_XXX and > COPY_DEST_XXX accordingly. > 2. Support custom format for both COPY TO and COPY FROM. > 3. Expose necessary helper functions such as CopySendEndOfRow(). > 4. Add CopyFromSkipErrorRow(). > 5. Documentation. The attached v39 patch set uses the followings: 0001: Create copyto_internal.h and change COPY_XXX to COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. (Same as 1. in your suggestion) 0002: Support custom format for both COPY TO and COPY FROM. (Same as 2. in your suggestion) 0003: Expose necessary helper functions such as CopySendEndOfRow() and add CopyFromSkipErrorRow(). (3. + 4. in your suggestion) 0004: Define handler functions for built-in formats. (Not included in your suggestion) 0005: Documentation. (WIP) (Same as 5. in your suggestion) We can merge 0001 quickly, right? Thanks, -- kou ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-29 05:37 ` Masahiko Sawada <[email protected]> 2025-03-29 05:57 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-29 08:57 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2 siblings, 2 replies; 24+ messages in thread From: Masahiko Sawada @ 2025-03-29 05:37 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Mar 26, 2025 at 8:28 PM Sutou Kouhei <[email protected]> wrote: > > > We need more regression tests for handling the given format name. For example, > > > > - more various input patterns. > > - a function with the specified format name exists but it returns an > > unexpected Node. > > - looking for a handler function in a different namespace. > > etc. > > I've added the following tests: > > * Wrong input type handler without namespace > * Wrong input type handler with namespace > * Wrong return type handler without namespace > * Wrong return type handler with namespace > * Wrong return value (Copy*Routine isn't returned) handler without namespace > * Wrong return value (Copy*Routine isn't returned) handler with namespace > * Nonexistent handler > * Invalid qualified name > * Valid handler without namespace and without search_path > * Valid handler without namespace and with search_path > * Valid handler with namespace Probably we can merge these newly added four files into one .sql file? Also we need to add some comments to describe what these queries test. For example, it's not clear to me at a glance what queries in no-schema.sql are going to test as there is no comment there. > > 0002 also includes this. > > > I think that we should accept qualified names too as the format name > > like tablesample does. That way, different extensions implementing the > > same format can be used. > > Implemented. It's implemented after parsing SQL. Is it OK? > (It seems that tablesample does it in parsing SQL.) I think it's okay. One problem in the following chunk I can see is: + qualified_format = stringToQualifiedNameList(format, NULL); + DeconstructQualifiedName(qualified_format, &schema, &fmt); + if (!schema || strcmp(schema, "pg_catalog") == 0) + { + if (strcmp(fmt, "csv") == 0) + opts_out->csv_mode = true; + else if (strcmp(fmt, "binary") == 0) + opts_out->binary = true; + } Non-qualified names depend on the search_path value so it's not necessarily a built-in format. If the user specifies 'csv' with seach_patch = 'myschema, pg_catalog', the COPY command unnecessarily sets csv_mode true. I think we can instead check if the retrieved handler function's OID matches the built-in formats' ones. Also, it's weired to me that cstate has csv_mode and binary fields even though the format should have already been known by the callback functions. Regarding the documentation for the existing options, it says "... only when not using XXX format." some places, where XXX can be replaced with binary or CSV. Once we support custom formats, 'non-CSV mode' would actually include custom formats as well, so we need to update the description too. > > > --- > > +static void > > +CopyToOneRow(CopyToState cstate, TupleTableSlot *slot) > > +{ > > + ereport(NOTICE, (errmsg("CopyToOneRow: tts_nvalid=%u", > > slot->tts_nvalid))); > > +} > > > > Similar to the above comment, the field name 'tts_nvalid' might also > > be changed in the future, let's use another name. > > Hmm. If the field name is changed, we need to change this > code. Yes, but if we use independe name in the NOTICE message we would not need to update the expected files. > > > --- > > +/* > > + * Export CopySendEndOfRow() for extensions. We want to keep > > + * CopySendEndOfRow() as a static function for > > + * optimization. CopySendEndOfRow() calls in this file may be optimized by a > > + * compiler. > > + */ > > +void > > +CopyToStateFlush(CopyToState cstate) > > +{ > > + CopySendEndOfRow(cstate); > > +} > > > > Is there any reason to use a different name for public functions? > > In this patch set, I use "CopyFrom"/"CopyTo" prefixes for > public APIs for custom COPY FORMAT handler extensions. It > will help understanding related APIs. Is it strange in > PostgreSQL? I see your point. Probably we need to find a better name as the name CopyToStateFlush doesn't sound well like this API should be called only once at the end of a row (IOW user might try to call it multiple times to 'flush' the state while processing a row). How about CopyToEndOfRow()? > > > --- > > + /* For custom format implementation */ > > + void *opaque; /* private space */ > > > > How about renaming 'private'? > > We should not use "private" because it's a keyword in > C++. If we use "private" here, we can't include this file > from C++ code. Understood. > > > --- > > I've not reviewed the documentation patch yet but I think the patch > > seems to miss the updates to the description of the FORMAT option in > > the COPY command section. > > I defer this for now. We can revisit the last documentation > patch after we finalize our API. (Or could someone help us?) > > > I think we can reorganize the patch set as follows: > > > > 1. Create copyto_internal.h and change COPY_XXX to COPY_SOURCE_XXX and > > COPY_DEST_XXX accordingly. > > 2. Support custom format for both COPY TO and COPY FROM. > > 3. Expose necessary helper functions such as CopySendEndOfRow(). > > 4. Add CopyFromSkipErrorRow(). > > 5. Documentation. > > The attached v39 patch set uses the followings: > > 0001: Create copyto_internal.h and change COPY_XXX to > COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. > (Same as 1. in your suggestion) > 0002: Support custom format for both COPY TO and COPY FROM. > (Same as 2. in your suggestion) > 0003: Expose necessary helper functions such as CopySendEndOfRow() > and add CopyFromSkipErrorRow(). > (3. + 4. in your suggestion) > 0004: Define handler functions for built-in formats. > (Not included in your suggestion) > 0005: Documentation. (WIP) > (Same as 5. in your suggestion) Can we merge 0002 and 0004? > We can merge 0001 quickly, right? I don't think it makes sense to push only 0001 as it's a completely preliminary patch for subsequent patches. It would be prudent to push it once other patches are ready too. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 05:37 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-29 05:57 ` David G. Johnston <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: David G. Johnston @ 2025-03-29 05:57 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Friday, March 28, 2025, Masahiko Sawada <[email protected]> wrote: > > > One problem in the following chunk I can see is: > > + qualified_format = stringToQualifiedNameList(format, NULL); > + DeconstructQualifiedName(qualified_format, &schema, &fmt); > + if (!schema || strcmp(schema, "pg_catalog") == 0) > + { > + if (strcmp(fmt, "csv") == 0) > + opts_out->csv_mode = true; > + else if (strcmp(fmt, "binary") == 0) > + opts_out->binary = true; > + } > > Non-qualified names depend on the search_path value so it's not > necessarily a built-in format. If the user specifies 'csv' with > seach_patch = 'myschema, pg_catalog', the COPY command unnecessarily > sets csv_mode true. I think we can instead check if the retrieved > handler function's OID matches the built-in formats' ones. Also, it's > weired to me that cstate has csv_mode and binary fields even though > the format should have already been known by the callback functions. > I considered it a feature that a schema-less reference to text, csv, or binary always resolves to the core built-in handlers. As does an unspecified format default of text. To use an extension that chooses to override that format name would require schema qualification. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 05:37 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-29 08:57 ` Sutou Kouhei <[email protected]> 2025-03-29 16:12 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-31 19:35 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 1 sibling, 2 replies; 24+ messages in thread From: Sutou Kouhei @ 2025-03-29 08:57 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAD21AoBKMNsO+b6wahb6847xwFci1JCfV+JykoMziVgiFxB6cw@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 28 Mar 2025 22:37:03 -0700, Masahiko Sawada <[email protected]> wrote: >> I've added the following tests: >> >> * Wrong input type handler without namespace >> * Wrong input type handler with namespace >> * Wrong return type handler without namespace >> * Wrong return type handler with namespace >> * Wrong return value (Copy*Routine isn't returned) handler without namespace >> * Wrong return value (Copy*Routine isn't returned) handler with namespace >> * Nonexistent handler >> * Invalid qualified name >> * Valid handler without namespace and without search_path >> * Valid handler without namespace and with search_path >> * Valid handler with namespace > > Probably we can merge these newly added four files into one .sql file? I know that src/test/regress/sql/ uses this style (one .sql file includes many test patterns in one large category). I understand that the style is preferable in src/test/regress/sql/ because src/test/regress/sql/ has tests for many categories. But do we need to follow the style in src/test/modules/*/sql/ too? If we use the style in src/test/modules/*/sql/, we need to have only one .sql in src/test/modules/*/sql/ because src/test/modules/*/ are for each category. And the current .sql per sub-category style is easy to debug (at least for me). For example, if we try qualified name cases on debugger, we can use "\i sql/schema.sql" instead of extracting target statements from .sql that includes many unrelated statements. (Or we can use "\i sql/all.sql" and many GDB "continue"s.) BTW, it seems that src/test/modules/test_ddl_deparse/sql/ uses .sql per sub-category style. Should we use one .sql file for sql/test/modules/test_copy_format/sql/? If it's required for merging this patch set, I'll do it. > Also we need to add some comments to describe what these queries test. > For example, it's not clear to me at a glance what queries in > no-schema.sql are going to test as there is no comment there. Hmm. You refer no_schema.sql in 0002, right? ---- CREATE EXTENSION test_copy_format; CREATE TABLE public.test (a smallint, b integer, c bigint); INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); \. COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); DROP TABLE public.test; DROP EXTENSION test_copy_format; ---- In general, COPY FORMAT tests focus on "COPY FROM WITH (FORMAT ...)" and "COPY TO WITH (FORMAT ...)". And the file name "no_schema" shows that it doesn't use qualified name. Based on this, I feel that the above content is very straightforward without any comment. What should we add as comments? For example, do we need the following comments? ---- -- This extension includes custom COPY handler: test_copy_format CREATE EXTENSION test_copy_format; -- Test data CREATE TABLE public.test (a smallint, b integer, c bigint); INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); -- Use custom COPY handler, test_copy_format, without -- schema for FROM. COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); \. -- Use custom COPY handler, test_copy_format, without -- schema for TO. COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); -- Cleanup DROP TABLE public.test; DROP EXTENSION test_copy_format; ---- > One problem in the following chunk I can see is: > > + qualified_format = stringToQualifiedNameList(format, NULL); > + DeconstructQualifiedName(qualified_format, &schema, &fmt); > + if (!schema || strcmp(schema, "pg_catalog") == 0) > + { > + if (strcmp(fmt, "csv") == 0) > + opts_out->csv_mode = true; > + else if (strcmp(fmt, "binary") == 0) > + opts_out->binary = true; > + } > > Non-qualified names depend on the search_path value so it's not > necessarily a built-in format. If the user specifies 'csv' with > seach_patch = 'myschema, pg_catalog', the COPY command unnecessarily > sets csv_mode true. I think that we should always use built-in COPY handlers for (not-qualified) "text", "csv" and "binary" for compatibility. If we allow custom COPY handlers for (not-qualified) "text", "csv" and "binary", pg_dump or existing dump may be broken. Because we must use the same COPY handler when we dump (COPY TO) and we restore (COPY FROM). BTW, the current implementation always uses pg_catalog.{text,csv,binary} for (not-qualified) "text", "csv" and "binary" even when there are myschema.{text,csv,binary}. See src/test/modules/test_copy_format/sql/builtin.sql. But I haven't looked into it why... > I think we can instead check if the retrieved > handler function's OID matches the built-in formats' ones. I agree that the approach is clear than the current implementation. I'll use it when I create the next patch set. > Also, it's > weired to me that cstate has csv_mode and binary fields even though > the format should have already been known by the callback functions. You refer CopyFomratOptions::{csv_mode,binary} not Copy{To,From}StateData, right? And you suggest that we should replace all opts.csv_mode and opts.binary with opts.handler == F_CSV and opts.handler == F_BINARY, right? We can do it but I suggest that we do it as a refactoring (or cleanup) in a separated patch for easy to review. > Regarding the documentation for the existing options, it says "... > only when not using XXX format." some places, where XXX can be > replaced with binary or CSV. Once we support custom formats, 'non-CSV > mode' would actually include custom formats as well, so we need to > update the description too. I agree with you. >> > --- >> > +/* >> > + * Export CopySendEndOfRow() for extensions. We want to keep >> > + * CopySendEndOfRow() as a static function for >> > + * optimization. CopySendEndOfRow() calls in this file may be optimized by a >> > + * compiler. >> > + */ >> > +void >> > +CopyToStateFlush(CopyToState cstate) >> > +{ >> > + CopySendEndOfRow(cstate); >> > +} >> > >> > Is there any reason to use a different name for public functions? >> >> In this patch set, I use "CopyFrom"/"CopyTo" prefixes for >> public APIs for custom COPY FORMAT handler extensions. It >> will help understanding related APIs. Is it strange in >> PostgreSQL? > > I see your point. Probably we need to find a better name as the name > CopyToStateFlush doesn't sound well like this API should be called > only once at the end of a row (IOW user might try to call it multiple > times to 'flush' the state while processing a row). How about > CopyToEndOfRow()? CopyToStateFlush() can be called multiple times in a row. It can also be called only once with multiple rows. Because it just flushes the current buffer. Existing CopySendEndOfRow() is called at the end of a row. (Buffer is flushed at the end of row.) So I think that the "EndOfRow" was chosen. Some custom COPY handlers may not be row based. For example, Apache Arrow COPY handler doesn't flush buffer for each row. So, we should provide "flush" API not "end of row" API for extensibility. >> The attached v39 patch set uses the followings: >> >> 0001: Create copyto_internal.h and change COPY_XXX to >> COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. >> (Same as 1. in your suggestion) >> 0002: Support custom format for both COPY TO and COPY FROM. >> (Same as 2. in your suggestion) >> 0003: Expose necessary helper functions such as CopySendEndOfRow() >> and add CopyFromSkipErrorRow(). >> (3. + 4. in your suggestion) >> 0004: Define handler functions for built-in formats. >> (Not included in your suggestion) >> 0005: Documentation. (WIP) >> (Same as 5. in your suggestion) > > Can we merge 0002 and 0004? Can we do it when we merge this patch set if it's still desirable at the time? Because: * I think that separated 0002 and 0004 patches are easier to review than squashed 0002 and 0004 patch. * I still think that someone may don't like defining COPY handlers for built-in formats. If we don't define COPY handlers for built-in formats finally, we can just drop 0004. >> We can merge 0001 quickly, right? > > I don't think it makes sense to push only 0001 as it's a completely > preliminary patch for subsequent patches. It would be prudent to push > it once other patches are ready too. Hmm. I feel that 0001 is a refactoring category patch like merged patches. In general, distinct enum value names are easier to understand. BTW, does the "other patches" include the documentation patch...? Thanks, -- kou ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 05:37 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-29 08:57 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-29 16:12 ` David G. Johnston <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: David G. Johnston @ 2025-03-29 16:12 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Sat, Mar 29, 2025 at 1:57 AM Sutou Kouhei <[email protected]> wrote: > * I still think that someone may don't like defining COPY > handlers for built-in formats. If we don't define COPY > handlers for built-in formats finally, we can just drop > 0004. > We should (and usually do) dog-food APIs when reasonable and this situation seems quite reasonable. I'd push back quite a bit about publishing this without any internal code leveraging it. > >> We can merge 0001 quickly, right? > > > > I don't think it makes sense to push only 0001 as it's a completely > > preliminary patch for subsequent patches. It would be prudent to push > > it once other patches are ready too. > > Hmm. I feel that 0001 is a refactoring category patch like > merged patches. In general, distinct enum value names are > easier to understand. > > I'm for pushing 0001. We've had copyfrom_internal.h for a while now and this seems like a simple refactor to make that area of the code cleaner via symmetry. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 05:37 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-29 08:57 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-31 19:35 ` Masahiko Sawada <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: Masahiko Sawada @ 2025-03-31 19:35 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Sat, Mar 29, 2025 at 1:57 AM Sutou Kouhei <[email protected]> wrote: > > Hi, > > In <CAD21AoBKMNsO+b6wahb6847xwFci1JCfV+JykoMziVgiFxB6cw@mail.gmail.com> > "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 28 Mar 2025 22:37:03 -0700, > Masahiko Sawada <[email protected]> wrote: > > >> I've added the following tests: > >> > >> * Wrong input type handler without namespace > >> * Wrong input type handler with namespace > >> * Wrong return type handler without namespace > >> * Wrong return type handler with namespace > >> * Wrong return value (Copy*Routine isn't returned) handler without namespace > >> * Wrong return value (Copy*Routine isn't returned) handler with namespace > >> * Nonexistent handler > >> * Invalid qualified name > >> * Valid handler without namespace and without search_path > >> * Valid handler without namespace and with search_path > >> * Valid handler with namespace > > > > Probably we can merge these newly added four files into one .sql file? > > I know that src/test/regress/sql/ uses this style (one .sql > file includes many test patterns in one large category). I > understand that the style is preferable in > src/test/regress/sql/ because src/test/regress/sql/ has > tests for many categories. > > But do we need to follow the style in > src/test/modules/*/sql/ too? If we use the style in > src/test/modules/*/sql/, we need to have only one .sql in > src/test/modules/*/sql/ because src/test/modules/*/ are for > each category. > > And the current .sql per sub-category style is easy to debug > (at least for me). For example, if we try qualified name > cases on debugger, we can use "\i sql/schema.sql" instead of > extracting target statements from .sql that includes many > unrelated statements. (Or we can use "\i sql/all.sql" and > many GDB "continue"s.) > > BTW, it seems that src/test/modules/test_ddl_deparse/sql/ > uses .sql per sub-category style. Should we use one .sql > file for sql/test/modules/test_copy_format/sql/? If it's > required for merging this patch set, I'll do it. I'm not sure that the regression test queries are categorized in the same way as in test_ddl_deparse. While the former have separate .sql files for different types of inputs (valid inputs and invalid inputs etc.) , which seems finer graind, the latter has .sql files for each DDL command. Most of the queries under test_copy_format/sql verifies the input patterns of the FORMAT option. I find that the regression tests included in that directory probably should focus on testing new callback APIs and some regression tests for FORMAT option handling can be moved into the normal regression test suite (e.g., in copy.sql or a new file like copy_format.sql). IIUC testing for invalid input patterns can be done even without creating artificial wrong handler functions. > > > Also we need to add some comments to describe what these queries test. > > For example, it's not clear to me at a glance what queries in > > no-schema.sql are going to test as there is no comment there. > > Hmm. You refer no_schema.sql in 0002, right? > > ---- > CREATE EXTENSION test_copy_format; > CREATE TABLE public.test (a smallint, b integer, c bigint); > INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); > COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); > \. > COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); > DROP TABLE public.test; > DROP EXTENSION test_copy_format; > ---- > > In general, COPY FORMAT tests focus on "COPY FROM WITH > (FORMAT ...)" and "COPY TO WITH (FORMAT ...)". And the file > name "no_schema" shows that it doesn't use qualified > name. Based on this, I feel that the above content is very > straightforward without any comment. > > What should we add as comments? For example, do we need the > following comments? > > ---- > -- This extension includes custom COPY handler: test_copy_format > CREATE EXTENSION test_copy_format; > -- Test data > CREATE TABLE public.test (a smallint, b integer, c bigint); > INSERT INTO public.test VALUES (1, 2, 3), (12, 34, 56), (123, 456, 789); > -- Use custom COPY handler, test_copy_format, without > -- schema for FROM. > COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); > \. > -- Use custom COPY handler, test_copy_format, without > -- schema for TO. > COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); > -- Cleanup > DROP TABLE public.test; > DROP EXTENSION test_copy_format; > ---- I'd like to see in the comment what the tests expect. Taking the following queries as an example, COPY public.test FROM stdin WITH (FORMAT 'test_copy_format'); \. COPY public.test TO stdout WITH (FORMAT 'test_copy_format'); it would help readers understand the test case better if we have a comment like for example: -- Specify the custom format name without schema. We test if both -- COPY TO and COPY FROM can find the correct handler function -- in public schema. > > > One problem in the following chunk I can see is: > > > > + qualified_format = stringToQualifiedNameList(format, NULL); > > + DeconstructQualifiedName(qualified_format, &schema, &fmt); > > + if (!schema || strcmp(schema, "pg_catalog") == 0) > > + { > > + if (strcmp(fmt, "csv") == 0) > > + opts_out->csv_mode = true; > > + else if (strcmp(fmt, "binary") == 0) > > + opts_out->binary = true; > > + } > > > > Non-qualified names depend on the search_path value so it's not > > necessarily a built-in format. If the user specifies 'csv' with > > seach_patch = 'myschema, pg_catalog', the COPY command unnecessarily > > sets csv_mode true. > > I think that we should always use built-in COPY handlers for > (not-qualified) "text", "csv" and "binary" for > compatibility. If we allow custom COPY handlers for > (not-qualified) "text", "csv" and "binary", pg_dump or > existing dump may be broken. Because we must use the same > COPY handler when we dump (COPY TO) and we restore (COPY > FROM). I agreed. > > BTW, the current implementation always uses > pg_catalog.{text,csv,binary} for (not-qualified) "text", > "csv" and "binary" even when there are > myschema.{text,csv,binary}. See > src/test/modules/test_copy_format/sql/builtin.sql. But I > haven't looked into it why... Sorry, I don't follow that. IIUC test_copy_format extension doesn't create a handler function in myschema schema, and SQLs in builtin.sql seem to work as expected (specifying a non-qualified built-in format unconditionally uses the built-in format). > > > Also, it's > > weired to me that cstate has csv_mode and binary fields even though > > the format should have already been known by the callback functions. > > You refer CopyFomratOptions::{csv_mode,binary} not > Copy{To,From}StateData, right? Yes. I referred to the wrong one. > And you suggest that we > should replace all opts.csv_mode and opts.binary with > opts.handler == F_CSV and opts.handler == F_BINARY, right? > > We can do it but I suggest that we do it as a refactoring > (or cleanup) in a separated patch for easy to review. I think that csv_mode and binary are used mostly in ProcessCopyOptions() so probably we can use local variables for that. I find there are two other places where to use csv_mode: NextCopyFromRawFields() and CopyToTextLikeStart(). I think we can simply check the handler function's OID there, or we can define macros like COPY_FORMAT_IS_TEXT/CSV/BINARY checking the OID and use them there. > > >> > --- > >> > +/* > >> > + * Export CopySendEndOfRow() for extensions. We want to keep > >> > + * CopySendEndOfRow() as a static function for > >> > + * optimization. CopySendEndOfRow() calls in this file may be optimized by a > >> > + * compiler. > >> > + */ > >> > +void > >> > +CopyToStateFlush(CopyToState cstate) > >> > +{ > >> > + CopySendEndOfRow(cstate); > >> > +} > >> > > >> > Is there any reason to use a different name for public functions? > >> > >> In this patch set, I use "CopyFrom"/"CopyTo" prefixes for > >> public APIs for custom COPY FORMAT handler extensions. It > >> will help understanding related APIs. Is it strange in > >> PostgreSQL? > > > > I see your point. Probably we need to find a better name as the name > > CopyToStateFlush doesn't sound well like this API should be called > > only once at the end of a row (IOW user might try to call it multiple > > times to 'flush' the state while processing a row). How about > > CopyToEndOfRow()? > > CopyToStateFlush() can be called multiple times in a row. It > can also be called only once with multiple rows. Because it > just flushes the current buffer. > > Existing CopySendEndOfRow() is called at the end of a > row. (Buffer is flushed at the end of row.) So I think that > the "EndOfRow" was chosen. > > Some custom COPY handlers may not be row based. For example, > Apache Arrow COPY handler doesn't flush buffer for each row. > So, we should provide "flush" API not "end of row" API for > extensibility. Okay, understood. > > >> We can merge 0001 quickly, right? > > > > I don't think it makes sense to push only 0001 as it's a completely > > preliminary patch for subsequent patches. It would be prudent to push > > it once other patches are ready too. > > Hmm. I feel that 0001 is a refactoring category patch like > merged patches. In general, distinct enum value names are > easier to understand. Right, but the patches that have already been merged contributed to speed up COPY commands, but 0001 patch also introduces copyto_internal.h, which is not used by anyone in a case where the custom copy format patch is not merged. Without adding copyto_internal.h changing enum value names less makes sense to me. > BTW, does the "other patches" include the documentation > patch...? I think that when pushing the main custom COPY format patch, we need to include the documentation changes into it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-29 16:48 ` David G. Johnston <[email protected]> 2025-03-30 02:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-31 18:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2 siblings, 2 replies; 24+ messages in thread From: David G. Johnston @ 2025-03-29 16:48 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Mar 26, 2025 at 8:28 PM Sutou Kouhei <[email protected]> wrote: > > The attached v39 patch set uses the followings: > > 0001: Create copyto_internal.h and change COPY_XXX to > COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. > (Same as 1. in your suggestion) > 0002: Support custom format for both COPY TO and COPY FROM. > (Same as 2. in your suggestion) > 0003: Expose necessary helper functions such as CopySendEndOfRow() > and add CopyFromSkipErrorRow(). > (3. + 4. in your suggestion) > 0004: Define handler functions for built-in formats. > (Not included in your suggestion) > 0005: Documentation. (WIP) > (Same as 5. in your suggestion) > > I don't think this module should be responsible for testing the validity of "qualified names in a string literal" behavior. Having some of the tests use a schema qualification, and I'd suggest explicit double-quoting/case-folding, wouldn't hurt just to demonstrate it's possible, and how extensions should be referenced, but definitely don't need tests to prove the broken cases are indeed broken. This relies on an existing API that has its own tests. It is definitely pointlessly redundant to have 6 tests that only differ from 6 other tests in their use of a schema qualification. I prefer keeping 0002 and 0004 separate. In particular, keeping the design choice of "unqualified internal format names ignore search_path" should stand out as its own commit. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 16:48 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> @ 2025-03-30 02:31 ` Sutou Kouhei <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: Sutou Kouhei @ 2025-03-30 02:31 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAKFQuwYF7VnYcS9dkfvdzt-dgftMB1DV0bjRcNC8-4iYGS+gjw@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 29 Mar 2025 09:48:22 -0700, "David G. Johnston" <[email protected]> wrote: > I don't think this module should be responsible for testing the validity of > "qualified names in a string literal" behavior. Having some of the tests > use a schema qualification, and I'd suggest explicit > double-quoting/case-folding, wouldn't hurt just to demonstrate it's > possible, and how extensions should be referenced, but definitely don't > need tests to prove the broken cases are indeed broken. This relies on an > existing API that has its own tests. It is definitely pointlessly > redundant to have 6 tests that only differ from 6 other tests in their use > of a schema qualification. You suggest the followings, right? 1. Add tests for "Schema.Name" with mixed cases 2. Remove the following 6 tests in src/test/modules/test_copy_format/sql/invalid.sql ---- COPY public.test FROM stdin WITH (FORMAT 'test_schema.test_copy_format_wrong_input_type'); COPY public.test TO stdout WITH (FORMAT 'test_schema.test_copy_format_wrong_input_type'); COPY public.test FROM stdin WITH (FORMAT 'test_schema.test_copy_format_wrong_return_type'); COPY public.test TO stdout WITH (FORMAT 'test_schema.test_copy_format_wrong_return_type'); COPY public.test FROM stdin WITH (FORMAT 'test_schema.test_copy_format_wrong_return_value'); COPY public.test TO stdout WITH (FORMAT 'test_schema.test_copy_format_wrong_return_value'); ---- because we have the following 6 tests: ---- SET search_path = public,test_schema; COPY public.test FROM stdin WITH (FORMAT 'test_copy_format_wrong_input_type'); COPY public.test TO stdout WITH (FORMAT 'test_copy_format_wrong_input_type'); COPY public.test FROM stdin WITH (FORMAT 'test_copy_format_wrong_return_type'); COPY public.test TO stdout WITH (FORMAT 'test_copy_format_wrong_return_type'); COPY public.test FROM stdin WITH (FORMAT 'test_copy_format_wrong_return_value'); COPY public.test TO stdout WITH (FORMAT 'test_copy_format_wrong_return_value'); RESET search_path; ---- 3. Remove the following tests because the behavior must be tested in other places: ---- COPY public.test FROM stdin WITH (FORMAT 'nonexistent'); COPY public.test TO stdout WITH (FORMAT 'nonexistent'); COPY public.test FROM stdin WITH (FORMAT 'invalid.qualified.name'); COPY public.test TO stdout WITH (FORMAT 'invalid.qualified.name'); ---- Does it miss something? 1.: There is no difference between single-quoting and double-quoting here. Because the information what quote was used for the given FORMAT value isn't remained here. Should we update gram.y? 2.: I don't have a strong opinion for it. If nobody objects it, I'll remove them. 3.: I don't have a strong opinion for it. If nobody objects it, I'll remove them. Thanks, -- kou ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 16:48 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> @ 2025-03-31 18:51 ` Masahiko Sawada <[email protected]> 2025-03-31 20:42 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Masahiko Sawada @ 2025-03-31 18:51 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers On Sat, Mar 29, 2025 at 9:49 AM David G. Johnston <[email protected]> wrote: > > On Wed, Mar 26, 2025 at 8:28 PM Sutou Kouhei <[email protected]> wrote: >> >> >> The attached v39 patch set uses the followings: >> >> 0001: Create copyto_internal.h and change COPY_XXX to >> COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. >> (Same as 1. in your suggestion) >> 0002: Support custom format for both COPY TO and COPY FROM. >> (Same as 2. in your suggestion) >> 0003: Expose necessary helper functions such as CopySendEndOfRow() >> and add CopyFromSkipErrorRow(). >> (3. + 4. in your suggestion) >> 0004: Define handler functions for built-in formats. >> (Not included in your suggestion) >> 0005: Documentation. (WIP) >> (Same as 5. in your suggestion) >> > > I prefer keeping 0002 and 0004 separate. In particular, keeping the design choice of "unqualified internal format names ignore search_path" should stand out as its own commit. What is the point of having separate commits for already-agreed design choices? I guess that it would make it easier to revert that decision. But I think it makes more sense that if we agree with "unqualified internal format names ignore search_path" the original commit includes that decision and describes it in the commit message. If we want to change that design based on the discussion later on, we can have a separate commit that makes that change and has the link to the discussion. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 16:48 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-31 18:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-31 20:42 ` David G. Johnston <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: David G. Johnston @ 2025-03-31 20:42 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers On Mon, Mar 31, 2025 at 11:52 AM Masahiko Sawada <[email protected]> wrote: > On Sat, Mar 29, 2025 at 9:49 AM David G. Johnston > <[email protected]> wrote: > > > > On Wed, Mar 26, 2025 at 8:28 PM Sutou Kouhei <[email protected]> wrote: > >> > >> > >> The attached v39 patch set uses the followings: > >> > >> 0001: Create copyto_internal.h and change COPY_XXX to > >> COPY_SOURCE_XXX and COPY_DEST_XXX accordingly. > >> (Same as 1. in your suggestion) > >> 0002: Support custom format for both COPY TO and COPY FROM. > >> (Same as 2. in your suggestion) > >> 0003: Expose necessary helper functions such as CopySendEndOfRow() > >> and add CopyFromSkipErrorRow(). > >> (3. + 4. in your suggestion) > >> 0004: Define handler functions for built-in formats. > >> (Not included in your suggestion) > >> 0005: Documentation. (WIP) > >> (Same as 5. in your suggestion) > >> > > > > I prefer keeping 0002 and 0004 separate. In particular, keeping the > design choice of "unqualified internal format names ignore search_path" > should stand out as its own commit. > > What is the point of having separate commits for already-agreed design > choices? I guess that it would make it easier to revert that decision. > But I think it makes more sense that if we agree with "unqualified > internal format names ignore search_path" the original commit includes > that decision and describes it in the commit message. If we want to > change that design based on the discussion later on, we can have a > separate commit that makes that change and has the link to the > discussion. > Fair. Comment withdrawn. Though I was referring to the WIP patches; I figured the final patch would squash this all together in any case. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-31 17:05 ` David G. Johnston <[email protected]> 2 siblings, 0 replies; 24+ messages in thread From: David G. Johnston @ 2025-03-31 17:05 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Mar 26, 2025 at 8:28 PM Sutou Kouhei <[email protected]> wrote: > > > --- > > +static const CopyFromRoutine CopyFromRoutineTestCopyFormat = { > > + .type = T_CopyFromRoutine, > > + .CopyFromInFunc = CopyFromInFunc, > > + .CopyFromStart = CopyFromStart, > > + .CopyFromOneRow = CopyFromOneRow, > > + .CopyFromEnd = CopyFromEnd, > > +}; > > > > In trying to document the current API I'm strongly disliking it. Namely, the amount of internal code an extension needs to care about/copy-paste to create a working handler. Presently, pg_type defines text and binary I/O routines and the text/csv formats use the text I/O while binary uses binary I/O - for all attributes. The CopyFromInFunc API allows for each attribute to somehow have its I/O format individualized. But I don't see how that is practical or useful, and it adds burden on API users. I suggest we remove both .CopyFromInFunc and .CopyFromStart/End and add a property to CopyFromRoutine (.ioMode?) with values of either Copy_IO_Text or Copy_IO_Binary and then just branch to either: CopyFromTextLikeInFunc & CopyFromTextLikeStart/End or CopyFromBinaryInFunc & CopyFromStart/End So, in effect, the only method an extension needs to write is converting to/from the 'serialized' form to the text/binary form (text being near unanimous). In a similar manner, the amount of boilerplate within CopyFromOneRow seems undesirable from an API perspective. cstate->cur_attname = NameStr(att->attname); cstate->cur_attval = string; if (string != NULL) nulls[m] = false; if (cstate->defaults[m]) { /* We must have switched into the per-tuple memory context */ Assert(econtext != NULL); Assert(CurrentMemoryContext == econtext->ecxt_per_tuple_memory); values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]); } /* * If ON_ERROR is specified with IGNORE, skip rows with soft errors */ else if (!InputFunctionCallSafe(&in_functions[m], string, typioparams[m], att->atttypmod, (Node *) cstate->escontext, &values[m])) { CopyFromSkipErrorRow(cstate); return true; } cstate->cur_attname = NULL; cstate->cur_attval = NULL; It seems to me that CopyFromOneRow could simply produce a *string collection, one cell per attribute, and NextCopyFrom could do all of the above on a for-loop over *string The pg_type and pg_proc catalogs are not extensible so the API can and should be limited to producing the, usually text, values that are ready to be passed into the text I/O routines and all the work to find and use types and functions left in the template code. I haven't looked at COPY TO but I am expecting much the same. The API should simply receive the content of the type I/O output routine (binary or text as it dictates) for each output attribute, by row, and be expected to take those values and produce a final output from them. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-25 00:45 ` Masahiko Sawada <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: Masahiko Sawada @ 2025-03-25 00:45 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Mar 19, 2025 at 6:25 PM Sutou Kouhei <[email protected]> wrote: > > Hi, > > In <CAKFQuwaMAFMHqxDXR=SxA0mDjdmntrwxZd2w=nSruLNFH-OzLw@mail.gmail.com> > "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 19 Mar 2025 17:49:49 -0700, > "David G. Johnston" <[email protected]> wrote: > > >> And could someone help (take over if possible) writing a > >> document for this feature? I'm not good at writing a > >> document in English... 0009 in the attached v37 patch set > >> has a draft of it. It's based on existing documents in > >> doc/src/sgml/ and *.h. > >> > >> > > I haven't touched the innards of the structs aside from changing > > programlisting to synopsis. And redoing the two section opening paragraphs > > to better integrate with the content in the chapter opening. > > > > The rest I kinda went to town on... > > Thanks!!! It's very helpful!!! > > I've applied your patch. 0009 is only changed. FYI I've implemented an extension to add JSON Lines format as a custom COPY format[1] to check the usability of the COPY format APIs. I think that the exposed APIs are fairly simple and minimum. I didn't find the deficiency and excess of exposed APIs for helping extensions but I find that it would be better to describe what the one-row callback should do to utilize the abstracted destination. For example, in order to use CopyToStateFlush() to write out to the destination, extensions should write the data to cstate->fe_msgbuf. We expose CopyToStateFlush() but not for any functions to write data there such as CopySendString(). It was a bit inconvenient to me but I managed to write the data directly there by #include'ing copyto_internal.h. Regards, [1] https://github.com/MasahikoSawada/pg_copy_jsonlines -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> @ 2025-03-22 00:31 ` David G. Johnston <[email protected]> 2025-03-22 05:07 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: David G. Johnston @ 2025-03-22 00:31 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers On Tue, Mar 18, 2025 at 7:56 PM Sutou Kouhei <[email protected]> wrote: > Hi, > > In <CAD21AoDU=bYRDDY8MzCXAfg4h9XTeTBdM-wVJaO1t4UcseCpuA@mail.gmail.com> > "Re: Make COPY format extendable: Extract COPY TO format > implementations" on Mon, 17 Mar 2025 13:50:03 -0700, > Masahiko Sawada <[email protected]> wrote: > > > I think that built-in formats also need to have their handler > > functions. This seems to be a conventional way for customizable > > features such as tablesample and access methods, and we can simplify > > this function. > > OK. 0008 in the attached v37 patch set does it. > > tl/dr; We need to exclude from our SQL function search any function that doesn't declare copy_handler as its return type. ("function text must return type copy_handler" is not an acceptable error message) We need to accept identifiers in FORMAT and parse the optional catalog, schema, and object name portions. (Restrict our function search to the named schema if provided.) Detail: Fun thing...(not sure how much of this is covered above: I do see, but didn't scour, the security discussion): -- create some poison create function public.text(internal) returns boolean language c as '/home/davidj/gotya/gotya', 'gotit'; CREATE FUNCTION -- inject it postgres=# set search_path to public,pg_catalog; SET -- watch it die postgres=# copy (select 1) to stdout (format text); ERROR: function text must return type copy_handler LINE 1: copy (select 1) to stdout (format text); I'm especially concerned about extensions here. We shouldn't be locating any SQL function that doesn't have a copy_handler return type. Unfortunately, LookupFuncName seems incapable of doing what we want here. I suggest we create a new lookup routine where we can specify the return argument type as a required element. That would cleanly mitigate the denial-of-service attack/accident vector demonstrated above (the text returning function should have zero impact on how this feature behaves). If someone does create a handler SQL function without using copy_handler return type we'd end up showing "COPY format 'david' not recognized" - a developer should be able to figure out they didn't put a correct return type on their handler function and that is why the system did not register it. A second concern is simply people wanting to name things the same; or, why namespaces were invented. Can we just accept a proper identifier after FORMAT so we can use schema-qualified names? (FORMAT "davescopyformat"."david") We can special case the internal schema-less names and internally force pg_catalog to avoid them being shadowed. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-22 00:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> @ 2025-03-22 05:07 ` Masahiko Sawada <[email protected]> 2025-03-22 05:23 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Masahiko Sawada @ 2025-03-22 05:07 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers On Fri, Mar 21, 2025 at 5:32 PM David G. Johnston <[email protected]> wrote: > > On Tue, Mar 18, 2025 at 7:56 PM Sutou Kouhei <[email protected]> wrote: >> >> Hi, >> >> In <CAD21AoDU=bYRDDY8MzCXAfg4h9XTeTBdM-wVJaO1t4UcseCpuA@mail.gmail.com> >> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 17 Mar 2025 13:50:03 -0700, >> Masahiko Sawada <[email protected]> wrote: >> >> > I think that built-in formats also need to have their handler >> > functions. This seems to be a conventional way for customizable >> > features such as tablesample and access methods, and we can simplify >> > this function. >> >> OK. 0008 in the attached v37 patch set does it. >> > > tl/dr; > > We need to exclude from our SQL function search any function that doesn't declare copy_handler as its return type. > ("function text must return type copy_handler" is not an acceptable error message) > > We need to accept identifiers in FORMAT and parse the optional catalog, schema, and object name portions. > (Restrict our function search to the named schema if provided.) > > Detail: > > Fun thing...(not sure how much of this is covered above: I do see, but didn't scour, the security discussion): > > -- create some poison > create function public.text(internal) returns boolean language c as '/home/davidj/gotya/gotya', 'gotit'; > CREATE FUNCTION > > -- inject it > postgres=# set search_path to public,pg_catalog; > SET > > -- watch it die > postgres=# copy (select 1) to stdout (format text); > ERROR: function text must return type copy_handler > LINE 1: copy (select 1) to stdout (format text); > > I'm especially concerned about extensions here. > > We shouldn't be locating any SQL function that doesn't have a copy_handler return type. Unfortunately, LookupFuncName seems incapable of doing what we want here. I suggest we create a new lookup routine where we can specify the return argument type as a required element. That would cleanly mitigate the denial-of-service attack/accident vector demonstrated above (the text returning function should have zero impact on how this feature behaves). If someone does create a handler SQL function without using copy_handler return type we'd end up showing "COPY format 'david' not recognized" - a developer should be able to figure out they didn't put a correct return type on their handler function and that is why the system did not register it. Just to be clear, the patch checks the function's return type before calling it: funcargtypes[0] = INTERNALOID; handlerOid = LookupFuncName(list_make1(makeString(format)), 1, funcargtypes, true); if (!OidIsValid(handlerOid)) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("COPY format \"%s\" not recognized", format), parser_errposition(pstate, defel->location))); /* check that handler has correct return type */ if (get_func_rettype(handlerOid) != COPY_HANDLEROID) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("function %s must return type %s", format, "copy_handler"), parser_errposition(pstate, defel->location))); So would changing the error message to like "COPY format 'text' not recognized" untangle your concern? FYI the same is true for TABLESAMPLE; it invokes a function with the specified method name and checks the returned Node type: =# select * from pg_class tablesample text (0); ERROR: function text must return type tsm_handler A difference between TABLESAMPLE and COPY format is that the former accepts a qualified name but the latter doesn't: =# create extension tsm_system_rows ; =# create schema s1; =# create function s1.system_rows(internal) returns void language c as 'tsm_system_rows.so', 'tsm_system_rows_handler'; =# \df *.system_rows List of functions Schema | Name | Result data type | Argument data types | Type --------+-------------+------------------+---------------------+------ public | system_rows | tsm_handler | internal | func s1 | system_rows | void | internal | func (2 rows) postgres(1:1194923)=# select count(*) from pg_class tablesample system_rows(0); count ------- 0 (1 row) postgres(1:1194923)=# select count(*) from pg_class tablesample s1.system_rows(0); ERROR: function s1.system_rows must return type tsm_handler > A second concern is simply people wanting to name things the same; or, why namespaces were invented. Yeah, I think that the custom COPY format should support qualified names at least. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-22 00:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-22 05:07 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> @ 2025-03-22 05:23 ` David G. Johnston <[email protected]> 2025-03-22 17:11 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: David G. Johnston @ 2025-03-22 05:23 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Friday, March 21, 2025, Masahiko Sawada <[email protected]> wrote: > On Fri, Mar 21, 2025 at 5:32 PM David G. Johnston > <[email protected]> wrote: > > > > On Tue, Mar 18, 2025 at 7:56 PM Sutou Kouhei <[email protected]> wrote: > >> > >> Hi, > >> > >> In <CAD21AoDU=bYRDDY8MzCXAfg4h9XTeTBdM-wVJaO1t4UcseCpuA@mail.gmail.com> > >> "Re: Make COPY format extendable: Extract COPY TO format > implementations" on Mon, 17 Mar 2025 13:50:03 -0700, > >> Masahiko Sawada <[email protected]> wrote: > >> > >> > I think that built-in formats also need to have their handler > >> > functions. This seems to be a conventional way for customizable > >> > features such as tablesample and access methods, and we can simplify > >> > this function. > >> > >> OK. 0008 in the attached v37 patch set does it. > >> > > > > tl/dr; > > > > We need to exclude from our SQL function search any function that > doesn't declare copy_handler as its return type. > > ("function text must return type copy_handler" is not an acceptable > error message) > > > > We need to accept identifiers in FORMAT and parse the optional catalog, > schema, and object name portions. > > (Restrict our function search to the named schema if provided.) > > > > Detail: > > > > Fun thing...(not sure how much of this is covered above: I do see, but > didn't scour, the security discussion): > > > > -- create some poison > > create function public.text(internal) returns boolean language c as > '/home/davidj/gotya/gotya', 'gotit'; > > CREATE FUNCTION > > > > -- inject it > > postgres=# set search_path to public,pg_catalog; > > SET > > > > -- watch it die > > postgres=# copy (select 1) to stdout (format text); > > ERROR: function text must return type copy_handler > > LINE 1: copy (select 1) to stdout (format text); > > > > I'm especially concerned about extensions here. > > > > We shouldn't be locating any SQL function that doesn't have a > copy_handler return type. Unfortunately, LookupFuncName seems incapable of > doing what we want here. I suggest we create a new lookup routine where we > can specify the return argument type as a required element. That would > cleanly mitigate the denial-of-service attack/accident vector demonstrated > above (the text returning function should have zero impact on how this > feature behaves). If someone does create a handler SQL function without > using copy_handler return type we'd end up showing "COPY format 'david' not > recognized" - a developer should be able to figure out they didn't put a > correct return type on their handler function and that is why the system > did not register it. > > Just to be clear, the patch checks the function's return type before > calling it: > > funcargtypes[0] = INTERNALOID; > handlerOid = LookupFuncName(list_make1(makeString(format)), 1, > > funcargtypes, true); > if (!OidIsValid(handlerOid)) > ereport(ERROR, > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > errmsg("COPY format \"%s\" not > recognized", format), > parser_errposition(pstate, > defel->location))); > > /* check that handler has correct return type */ > if (get_func_rettype(handlerOid) != COPY_HANDLEROID) > ereport(ERROR, > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > errmsg("function %s must return type %s", > format, "copy_handler"), > parser_errposition(pstate, > defel->location))); > > So would changing the error message to like "COPY format 'text' not > recognized" untangle your concern? In my example above copy should not fail at all. The text function created in public that returns Boolean would never be seen and the real one in pg_catalog would then be found and behave as expected. > > FYI the same is true for TABLESAMPLE; it invokes a function with the > specified method name and checks the returned Node type: > > =# select * from pg_class tablesample text (0); > ERROR: function text must return type tsm_handler Then this would benefit from the new function I suggest creating since it apparently has the same, IMO, bug. David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-22 00:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-22 05:07 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-22 05:23 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> @ 2025-03-22 17:11 ` David G. Johnston <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: David G. Johnston @ 2025-03-22 17:11 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Fri, Mar 21, 2025 at 10:23 PM David G. Johnston < [email protected]> wrote: > Then this would benefit from the new function I suggest creating since it > apparently has the same, IMO, bug. > > Concretely like I posted here: https://www.postgresql.org/message-id/[email protected]... David J. ^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2025-03-31 20:42 UTC | newest] Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-09-19 08:40 [PATCH v1 2/3] Replace magic "one" in tsvector binary receiving=0A= Denis Erokhin <[email protected]> 2025-03-03 19:06 Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-05 00:06 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-17 20:50 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-19 02:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-20 00:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-20 01:24 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-23 09:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-27 03:28 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 05:37 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-29 05:57 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-29 08:57 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-29 16:12 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-31 19:35 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-29 16:48 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-30 02:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]> 2025-03-31 18:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-31 20:42 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-31 17:05 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-25 00:45 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-22 00:31 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-22 05:07 ` Re: Make COPY format extendable: Extract COPY TO format implementations Masahiko Sawada <[email protected]> 2025-03-22 05:23 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]> 2025-03-22 17:11 ` Re: Make COPY format extendable: Extract COPY TO format implementations David G. Johnston <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox