Date: Tue, 30 Jan 2024 17:15:11 +0900 (JST)
Message-Id: <20240130.171511.2014195814665030502.kou@clear-code.com>
To: michael@paquier.xyz
Cc: sawada.mshk@gmail.com, zhjwpku@gmail.com, andrew@dunslane.net,
 nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <ZbijVn9_51mljMAG@paquier.xyz>
References: 
 <CAD21AoBmNiWwrspuedgAPgbAqsn7e7NoZYF6gNnYBf+gXEk9Mg@mail.gmail.com>
	<20240130.144531.1257430878438173740.kou@clear-code.com>
	<ZbijVn9_51mljMAG@paquier.xyz>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240130.171511.2014195814665030502.kou%40clear-code.com>
Precedence: bulk

Hi,

In <ZbijVn9_51mljMAG@paquier.xyz>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 30 Jan 2024 16:20:54 +0900,
  Michael Paquier <michael@paquier.xyz> wrote:

>>> +        if (!format_specified)
>>> +                /* Set the default format. */
>>> +                ProcessCopyOptionFormatTo(pstate, opts_out, cstate, NULL);
>>> +
>>> 
>>> I think we can pass "text" in this case instead of NULL. That way,
>>> ProcessCopyOptionFormatTo doesn't need to handle NULL case.
>> 
>> Yes, we can do it. But it needs a DefElem allocation. Is it
>> acceptable?
> 
> I don't think that there is a need for a DelElem at all here?

We use defel->location for an error message. (We don't need
to set location for the default "text" DefElem.)

>                                                                While I
> am OK with the choice of calling CopyToInit() in the
> ProcessCopyOption*() routines that exist to keep the set of callbacks
> local to copyto.c and copyfrom.c, I think that this should not bother
> about setting opts_out->csv_mode or opts_out->csv_mode but just set 
> the opts_out->{to,from}_routine callbacks.

OK. I'll keep opts_out->{csv_mode,binary} in copy.c.

>                  Now, all the Init() callbacks are empty for the
> in-core callbacks, so I think that we should just remove it entirely
> for now.  Let's keep the core patch a maximum simple.  It is always
> possible to build on top of it depending on what people need.  It's
> been mentioned that JSON would want that, but this also proves that we
> just don't care about that for all the in-core callbacks, as well.  I
> would choose a minimalistic design for now.

OK. Let's remove Init() callbacks from the first patch set.

> I would be really tempted to put my hands on this patch to put into
> shape a minimal set of changes because I'm caring quite a lot about
> the performance gains reported with the removal of the "if" checks in
> the per-row callbacks, and that's one goal of this thread quite
> independent on the extensibility.  Sutou-san, would you be OK with
> that?

Yes, sure.
(We want to focus on the performance gains in the first
patch set and then focus on extensibility again, right?)

For the purpose, I think that the v7 patch set is more
suitable than the v9 patch set. The v7 patch set doesn't
include Init() callbacks, custom options validation support
or extra Copy{In,Out}Response support. But the v7 patch set
misses the removal of the "if" checks in
NextCopyFromRawFields() that exists in the v9 patch set. I'm
not sure how much performance will improve by this but it
may be worth a try.

Can I prepare the v10 patch set as "the v7 patch set" + "the
removal of the "if" checks in NextCopyFromRawFields()"?
(+ reverting opts_out->{csv_mode,binary} changes in
ProcessCopyOptions().)


Thanks,
-- 
kou