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

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