Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael@paquier.xyz>
Cc: Sutou Kouhei <kou@clear-code.com>, sawada.mshk@gmail.com, zhjwpku@gmail.com, andrew@dunslane.net, nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Date: 2024-02-06T05:46:42Z
Lists: pgsql-hackers 
Hi,

On 2024-02-06 11:41:06 +0900, Michael Paquier wrote:
> On Mon, Feb 05, 2024 at 05:41:25PM -0800, Andres Freund wrote:
> > On 2024-02-06 10:01:36 +0900, Michael Paquier wrote:
> >> If you have concerns about that, I'm OK to revert, I'm not wedded to
> >> this level of control.  Note that I've actually seen *better*
> >> runtimes.
> > 
> > I'm somewhat worried that handling the different formats at that level will
> > make it harder to improve copy performance - it's quite attrociously slow
> > right now. The more we reduce the per-row/field overhead, the more the
> > dispatch overhead will matter.
> 
> Yep.  That's the hard part when it comes to design these callbacks.
> We don't want something too high level because this leads to more code
> duplication churns when someone wants to plug in its own routine set,
> and we don't want to be at a too low level because of the indirect
> calls as you said.  I'd like to think that the current CopyFromOneRow
> offers a good balance here, avoiding the "if" branch with the binary
> and non-binary paths.

One way to address code duplication is to use static inline helper functions
that do a lot of the work in a generic fashion, but where the compiler can
optimize the branches away, because it can do constant folding.


> >> If yes, then I'd assume that this shuts down the whole thread or that it
> >> needs a completely different approach, because we will multiply indirect
> >> function calls that can control how data is generated for each row, which is
> >> the original case that Sutou-san wanted to tackle.
> > 
> > I think it could be rescued fairly easily - remove the dispatch via
> > ->copy_attribute_out().  To avoid duplicating code you could use a static
> > inline function that's used with constant arguments by both csv and text mode.
> 
> Hmm.  So you basically mean to tweak the beginning of
> CopyToTextOneRow() and CopyToTextStart() so as copy_attribute_out is
> saved in a local variable outside of cstate and we'd save the "if"
> checked for each attribute.  If I got that right, it would mean
> something like the v13-0002 attached, on top of the v13-0001 of
> upthread.  Is that what you meant?

No - what I mean is that it doesn't make sense to have copy_attribute_out(),
as e.g. CopyToTextOneRow() already knows that it's dealing with text, so it
can directly call the right function. That does require splitting a bit more
between csv and text output, but I think that can be done without much
duplication.

Greetings,

Andres Freund

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.