Hi,
In <CAD21AoBLbmxT6xMHkDjfvBZEJUo1VbCd-vWhwX9WXbpMgaGW_A@mail.gmail.com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 23 Jun 2026 18:15:10 -0700,
Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for reviewing the patches! I've attached updated patches.
+1
I have only a few minor comments:
0002:
> --- /dev/null
> +++ b/src/backend/commands/copyapi.c
> +void
> +RegisterCopyCustomFormat(const char *name, const CopyToRoutine *to,
> + const CopyFromRoutine *from, ProcessOneCopyOptionFn option_fn)
How about using "const CopyCustomFormatEntry *" instead of
"to", "from" and "option_fn"? If we use
CopyCustomFormatEntry here, we don't need change the
signature of this function when we add more items.
> +const CopyCustomFormatEntry *
> +GetCopyCustomFormatRoutines(const char *name)
How about renaming this to "...FormatEntry" from
"...FormatRoutines"?
> --- a/src/include/commands/copy.h
> +++ b/src/include/commands/copy.h
> +#define CopyFormatIsBuiltins(format) ((format) != COPY_FORMAT_CUSTOM)
How about removing the last "s" ("...IsBuiltin") because
this processes only one format?
> + const struct CopyCustomFormatEntry *custom_format_ent;
It may be better that we don't abbreviate "_entry" to "_ent"
here for readability. It seems that we use this abbreviation
only in a few places:
$ git grep '_ent;' src/
src/backend/replication/logical/reorderbuffer.c: ReorderBufferTupleCidEnt *new_ent;
src/backend/utils/cache/catcache.c: CatCInProgress in_progress_ent;
src/backend/utils/cache/catcache.c: catcache_in_progress_stack = &in_progress_ent;
src/backend/utils/cache/catcache.c: CatCInProgress in_progress_ent;
src/backend/utils/cache/catcache.c: catcache_in_progress_stack = &in_progress_ent;
> I'll verify that the new API works well with an experimental custom
> copy format extension.
I think that we need to provide more APIs to read/write data
like we did in v40-0003 to implement a custom copy format
extension:
https://www.postgresql.org/message-id/flat/20250425.214534.1841428689427124725.kou%40clear-code.com
At least https://github.com/kou/pg-copy-arrow needs them.
Thanks,
--
kou
