Horton

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

Sutou Kouhei <kou@clear-code.com>

View thread View on postgr.es
From: Sutou Kouhei <kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, pgsql-hackers@postgresql.org
Date: 2024-11-25T02:06:20Z
Lists: 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

Commits

  1. Refactor Copy{From|To}GetRoutine() to use pass-by-reference argument.

  2. Refactor COPY FROM to use format callback functions.

  3. Refactor COPY TO to use format callback functions.

  4. Another try to fix BF failure introduced in commit ddd5f4f54a.

  5. Revert "Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM"

  6. Improve COPY TO performance when server and client encodings match

  7. Simplify signature of CopyAttributeOutCSV() in copyto.c

  8. Revert "Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO"

  9. Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO

  10. Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM

  11. Add progress reporting of skipped tuples during COPY FROM.

  12. pgbench: Add \syncpipeline

  13. meson: Make gzip and tar optional

  14. Export the external file reader used in COPY FROM as APIs.