Re: Make COPY format extendable: Extract COPY TO format implementations

On Mon, Jun 22, 2026 at 10:41 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> > - 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.

Agreed.

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

Agreed.

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

Agreed.

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

Agreed.

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

It would save just a few lines. It might be worth having the
initialization callback if it would enable extensions to do what
cannot be done with the current proposed callbacks.

Thank you for reviewing the patches! I've attached updated patches.
I'll verify that the new API works well with an experimental custom
copy format extension.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com