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