Date: Mon, 25 Nov 2024 11:06:20 +0900 (JST)
Message-Id: <20241125.110620.313152541320718947.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, 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: 
 <CAD21AoBNfKDbJnu-zONNpG820ZXYC0fuTSLrJ-UdRqU4qp2wog@mail.gmail.com>
References: 
 <CAD21AoA1s0nzjGU9t3N_uNdg3SZeOxXyH3rQfxYFEN3Y7JrKRQ@mail.gmail.com>
	<20241121.115531.1600295431613712107.kou@clear-code.com>
	<CAD21AoBNfKDbJnu-zONNpG820ZXYC0fuTSLrJ-UdRqU4qp2wog@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/20241125.110620.313152541320718947.kou%40clear-code.com>
Precedence: bulk

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