Date: Fri, 26 Jun 2026 12:21:50 +0900 (JST)
Message-Id: <20260626.122150.1718021693730851340.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: 
 <CAD21AoBLbmxT6xMHkDjfvBZEJUo1VbCd-vWhwX9WXbpMgaGW_A@mail.gmail.com>
References: 
 <CAD21AoCnA7vayZAOmwVqTSOyWfyBhyxH7mBb4UzjskF-eZ+_Jg@mail.gmail.com>
	<20260623.144149.439869848255136426.kou@clear-code.com>
	<CAD21AoBLbmxT6xMHkDjfvBZEJUo1VbCd-vWhwX9WXbpMgaGW_A@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/20260626.122150.1718021693730851340.kou%40clear-code.com>
Precedence: bulk

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