agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Sutou Kouhei <kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz
Cc: pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
Date: Mon, 25 Nov 2024 11:06:20 +0900 (JST)
Message-ID: <20241125.110620.313152541320718947.kou@clear-code.com> (raw)
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>
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#...
Thanks,
--
kou
view thread (340+ messages) latest in thread
Message-ID: <20241125.110620.313152541320718947.kou@clear-code.com>
Permalink: ../20241125.110620.313152541320718947.kou@clear-code.com/
Also on: postgresql.org/message-id/20241125.110620.313152541320718947.kou@clear-code.com
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: kou@clear-code.com, sawada.mshk@gmail.com, michael@paquier.xyz
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
In-Reply-To: <20241125.110620.313152541320718947.kou@clear-code.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox