public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Sutou Kouhei <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
Date: Mon, 5 Feb 2024 17:41:25 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Hi,

On 2024-02-06 10:01:36 +0900, Michael Paquier wrote:
> On Mon, Feb 05, 2024 at 10:21:18AM -0800, Andres Freund wrote:
> > Have you benchmarked the performance effects of 2889fd23be5 ? I'd not at all
> > be surprised if it lead to a measurable performance regression.
>
> Yes, I was looking at runtimes and some profiles around CopyOneRowTo()
> to see the effects that this has yesterday.  The principal point of
> contention is CopyOneRowTo() where the callback is called once per
> attribute, so more attributes stress it more.

Right.


> 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.



> [1]: https://www.postgresql.org/message-id/[email protected]
>
> > I think callbacks for individual attributes is the wrong approach - the
> > dispatch needs to happen at a higher level, otherwise there are too many
> > indirect function calls.
>
> Hmm.  Do you have concerns about v13 posted on [2] then?

As is I'm indeed not a fan. It imo doesn't make sense to have an indirect
dispatch for *both* ->copy_attribute_out *and* ->CopyToOneRow. After all, when
in ->CopyToOneRow for text, we could know that we need to call
CopyAttributeOutText etc.


> 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.

I think it might also be worth ensuring that future patches can move branches
like
	if (cstate->encoding_embeds_ascii)
	if (cstate->need_transcoding)
into the choice of per-row callback.


> The End, Start and In/OutFunc callbacks are called only once per query, so
> these don't matter AFAIU.

Right.

Greetings,

Andres Freund

view thread (6+ messages)  latest in thread

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
  In-Reply-To: <[email protected]>

* 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