Date: Tue, 23 Jun 2026 14:41:49 +0900 (JST)
Message-Id: <20260623.144149.439869848255136426.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: tomas@vondra.me, andres@anarazel.de, michael@paquier.xyz,
 david.g.johnston@gmail.com, tgl@sss.pgh.pa.us, zhjwpku@gmail.com,
 pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: 
 <CAD21AoCnA7vayZAOmwVqTSOyWfyBhyxH7mBb4UzjskF-eZ+_Jg@mail.gmail.com>
References: 
 <CAD21AoCLxUhQ0uBjDKXvCEtJBCfF13Ru_7u-Qrrsu+0PPUqcPQ@mail.gmail.com>
	<20260327.103640.1195140797832123502.kou@clear-code.com>
	<CAD21AoCnA7vayZAOmwVqTSOyWfyBhyxH7mBb4UzjskF-eZ+_Jg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20260623.144149.439869848255136426.kou%40clear-code.com>
Precedence: bulk

Hi,

Thanks for restarting this.

In <CAD21AoCnA7vayZAOmwVqTSOyWfyBhyxH7mBb4UzjskF-eZ+_Jg@mail.gmail.com>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 22 Jun 2026 18:06:07 -0700,
  Masahiko Sawada <sawada.mshk@gmail.com> wrote:

> After more thought, I'd like to keep the custom-format changes to the
> bare minimum and not disturb the existing built-in format processing.

+1

> Updated patches attached:
> 
> - 0001 moves CopyFromStateData and CopyToStateData to a new
> copy_state.h, so extensions can implement their routines without
> including the *_internal.h headers. It also drops file_fdw.c's
> dependency on copyfrom_internal.h.

+1

> - 0002 introduces the registration API and the opaque per-format
> pointer in both structs.

> --- /dev/null
> +++ b/src/backend/commands/copyapi.c

> +bool
> +GetCopyCustomFormatRoutines(const char *name, const CopyToRoutine **to,
> +							const CopyFromRoutine **from, ProcessOneOptionFn * option_fn)

How about returning CopyCustomFormatEntry instead? The
function name is "Get...Routines" but it also returns
ProcessOneOptionFn. "Get...Routines" is a bit strange.

> --- a/src/include/commands/copyapi.h
> +++ b/src/include/commands/copyapi.h

> @@ -102,4 +103,40 @@ typedef struct CopyFromRoutine
> ...
> +typedef bool (*ProcessOneOptionFn) (CopyFormatOptions *opts, bool is_from,
> +									DefElem *option);

How about adding "Copy" keyword to the type name such as
"ProcessOneCopyOptionFn" because this is only for COPY format?

> --- a/src/include/commands/copy.h
> +++ b/src/include/commands/copy.h

> @@ -58,7 +58,16 @@ typedef enum CopyFormat
> ...
> +#define CopyFormatBuiltins(format) ((format) != COPY_FORMAT_CUSTOM)

How about renaming this to CopyFormatIsBuiltin() or
something? "...Builtins" is a bit strange because this
returns a boolean.

> - 0003 adds a callback to validate the COPY options as a whole, called
> after all options are processed.

> --- a/src/include/commands/copyapi.h
> +++ b/src/include/commands/copyapi.h
> @@ -120,6 +120,15 @@ typedef struct CopyFromRoutine
> ...
> +typedef void (*ValidateOptionsFn) (CopyFormatOptions *opts, bool is_from);

How about adding "Copy" keyword like "ValidateCopyOptionsFn"?

> - 0004 adds the regression tests.

> --- /dev/null
> +++ b/src/test/modules/test_copy_custom_format/test_copy_custom_format.c

> @@ -0,0 +1,169 @@
> ...
> +TestCopyProcessOneOption(CopyFormatOptions *opts, bool is_from, DefElem *option)
> +{
> +	TestCopyOptions *t = (TestCopyOptions *) opts->format_private_opts;
> +
> +	if (t == NULL)
> +	{
> +		t = palloc0_object(TestCopyOptions);
> +		opts->format_private_opts = (void *) t;
> +	}

This is not a blocker but we may want to add
InitializeCopyOptions callback for this.


Thanks,
-- 
kou