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

Hi,

In <CAD21AoBNfKDbJnu-zONNpG820ZXYC0fuTSLrJ-UdRqU4qp2wog(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Nov 2024 13:01:06 -0800,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

>> @@ -1237,7 +1219,7 @@ CopyReadLine(CopyFromState cstate, bool is_csv)
>> /*
>> * CopyReadLineText - inner loop of CopyReadLine for text mode
>> */
>> -static pg_attribute_always_inline bool
>> +static bool
>> CopyReadLineText(CopyFromState cstate, bool is_csv)
>>
>> Is this an intentional change?
>> CopyReadLineText() has "bool in_csv".
>
> Yes, I'm not sure it's really necessary to make it inline since the
> benchmark results don't show much difference. Probably this is because
> the function has 'is_csv' in some 'if' branches but the compiler
> cannot optimize out the whole 'if' branches as most 'if' branches
> check 'is_csv' and other variables.

I see. If explicit "inline" isn't related to performance, we
don't need explicit "inline".

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

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

FYI: We discussed this in the past. For example:
https://www.postgresql.org/message-id/flat/20240115.152350.1128880926282754664.kou%40clear-code.com#1b523fb95e8fb46702f5568ae19e3649

Thanks,
--
kou