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

Hi,

In <20241125(dot)110620(dot)313152541320718947(dot)kou(at)clear-code(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 25 Nov 2024 11:06:20 +0900 (JST),
Sutou Kouhei <kou(at)clear-code(dot)com> wrote:

>> I've attached the v25 patches that squashed the minor changes I made
>> in v24 and incorporated all comments I got so far. I think these two
>> patches are in good shape. Could you rebase remaining patches on top
>> of them so that we can see the big picture of this feature?
>
> OK. I'll work on it.

I've attached the v26 patch set:

0001: It's same as 0001 in the v25 patch set.
0002: It's same as 0002 in the v25 patch set.
0003: It's same as 0003 in the v23 patch set.
This parses the "format" option and adds support for
custom TO handler.
0004: It's same as 0004 in the v23 patch set.
This exports CopyToStateData. But the following
enums/structs/functions aren't moved to copyapi.h from
copy.h:
* CopyHeaderChoice
* CopyOnErrorChoice
* CopyLogVerbosityChoice
* CopyFormatOptions
* copy_data_dest_cb()
0005: It's same as 0005 in the v23 patch set.
This adds missing APIs to implement custom TO handler
as an extension.
0006: It's same as 0008 in the v23 patch set.
This adds support for custom FROM handler.
0007: It's same as 0009 in the v23 patch set.
This exports CopyFromStateData.
0008: It's same as 0010 in the v23 patch set.
This adds missing APIs to implement custom FROM handler
as an extension.

I've also updated https://github.com/kou/pg-copy-arrow for
the current API.

I think that we can merge only 0001/0002 as the first
step. Because they don't change the current behavior and
they improve performance. We can merge other patches after
that.

>> Regarding exposing the structs such as CopyToStateData, v22-0004 patch
>> moves most of all copy-related structs to copyapi.h from copyto.c,
>> copyfrom_internal.h, and copy.h, which seems odd to me. I think we can
>> expose CopyToStateData (and related structs) in a new file
>> copyto_internal.h and keep other structs in the original header files.
>
> Custom COPY format extensions need to use
> CopyToStateData/CopyFromStateData. For example,
> CopyToStateData::rel is used to retrieve table schema. If we
> move CopyToStateData to copyto_internal.h not copyapi.h,
> custom COPY format extensions need to include
> copyto_internal.h. I feel that it's strange that extensions
> need to use internal headers.
>
> What is your real concern? If you don't want to export
> CopyToStateData/CopyFromStateData entirely, we can provide
> accessors only for some members of them.

The v26 patch set still exports
CopyToStateData/CopyFromStateData in copyapi.h not
copy{to,from}_internal.h. But I didn't move the following
enums/structs/functions:

* CopyHeaderChoice
* CopyOnErrorChoice
* CopyLogVerbosityChoice
* CopyFormatOptions
* copy_data_dest_cb()

What do you think about this approach?

Thanks,
--
kou