Re: Make COPY format extendable: Extract COPY TO format implementations - Mailing list pgsql-hackers

From Sutou Kouhei
Subject Re: Make COPY format extendable: Extract COPY TO format implementations
Date
Msg-id 20241125.110620.313152541320718947.kou@clear-code.com
Whole thread Raw
In response to Make COPY format extendable: Extract COPY TO format implementations  (Sutou Kouhei <kou@clear-code.com>)
Responses Re: Make COPY format extendable: Extract COPY TO format implementations
Re: Make COPY format extendable: Extract COPY TO format implementations
List pgsql-hackers
Hi,

In <CAD21AoBNfKDbJnu-zONNpG820ZXYC0fuTSLrJ-UdRqU4qp2wog@mail.gmail.com>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Nov 2024 13:01:06 -0800,
  Masahiko Sawada <sawada.mshk@gmail.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



pgsql-hackers by date:

Previous
From: Noah Misch
Date:
Subject: Re: Proper object locking for GRANT/REVOKE
Next
From: jian he
Date:
Subject: Re: POC, WIP: OR-clause support for indexes