Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 30 Jan 2024 17:37:35 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sutou Kouhei <kou@clear-code.com>
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
Message-ID: <Zbi1TwPfAvUpKqTd@paquier.xyz>
References: 
 <CAD21AoBmNiWwrspuedgAPgbAqsn7e7NoZYF6gNnYBf+gXEk9Mg@mail.gmail.com>
 <20240130.144531.1257430878438173740.kou@clear-code.com>
 <ZbijVn9_51mljMAG@paquier.xyz>
 <20240130.171511.2014195814665030502.kou@clear-code.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HtyyS77X0bduSMm8"
Content-Disposition: inline
In-Reply-To: <20240130.171511.2014195814665030502.kou@clear-code.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Zbi1TwPfAvUpKqTd%40paquier.xyz>
Precedence: bulk


--HtyyS77X0bduSMm8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 30, 2024 at 05:15:11PM +0900, Sutou Kouhei wrote:
> We use defel->location for an error message. (We don't need
> to set location for the default "text" DefElem.)

Yeah, but you should not need to have this error in the paths that set
the callback routines in opts_out if the same validation happens a few
lines before, in copy.c.

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

Yep, exactly, the numbers are too good to just ignore.  I don't want
to hijack the thread, but I am really worried about the complexities
this thread is getting into because we are trying to shape the
callbacks in the most generic way possible based on *two* use cases.
This is going to be a never-ending discussion.  I'd rather get some
simple basics, and then we can discuss if tweaking the callbacks is
really necessary or not.  Even after introducing the pg_proc lookups
to get custom callbacks.

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

Yeah..  The custom options don't seem like an absolute strong
requirement for the first shot with the callbacks or even the
possibility to retrieve the callbacks from a function call.  I mean,
you could provide some control with SET commands and a few GUCs, at
least, even if that would be strange.  Manipulations with a list of
DefElems is the intuitive way to have custom options at query level,
but we also have to guess the set of callbacks from this list of
DefElems coming from the query.  You see my point, I am not sure=20
if it would be the best thing to process twice the options, especially
when it comes to decide if a DefElem should be valid or not depending
on the callbacks used.  Or we could use a kind of "special" DefElem
where we could store a set of key:value fed to a callback :)

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

Yep, if I got it that would make sense to me.  If you can do that,
that would help quite a bit.  :)
--
Michael

--HtyyS77X0bduSMm8
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmW4tU8ACgkQnvQgOdby
QH0olQ/+OcL7PH4rHaO2TiMNDq5UMJ1mBjN3FeuOguteocJ1JHLTvHp1Wl62lhQ4
4kTCc+NBvJEn048w3X81d/fl76gvOXVOTxG3wsLGm+z4pOz3I/ahsjFsAXhBidUh
DZI4/5bxGkouiJt5FmwYWHiAzscJ8DYW2dwtVMiLBdXzsQBNRPinEj1lt+tjtLML
kjg3uDb1wCDlliez+eXRXsI4DcdY339CAkEAA6h3EAKdjVwAB3k8GnOF6l5OP4eZ
FOyOokNQVz7dtACvW1mXyfCbS9UtbB8ZFD+0D8SXQFDhQTEdVH7iZnakYGWcZigd
HUs3eEvMFty8e6E7+0iR0/Nl26QeTQ90lbQ5qmkoTLimrLPSYbt13tD2qz0opOXF
i/9gPqBPNBzNi4XDw1MScvoKntlCd7htDjRTCMkcHWANZiqNXp0OsZIzJFy1iCrS
maZ44RzF4b1qrH2eD6L8WSoTeRfNaFOBmcnCDQ9YTMI5Lj7JqwUaEFBw0nTGqVTN
MDy80T95qADGR85nhK1Foel4A6RcwLSjETXCtNLfndcgFIdHWQ9glNvoz8d1r14m
LJ9kmRwGhJ2EyulH0oDsE6gzimVj69IBwdRwbMT/Dgy9SiEzK0E1dD6VWKwgyQBk
xyFtUZxRQEbLKZZ1UXT3IXjangrtWslIWvXPG86HH61vkO6rXGo=
=Sc/7
-----END PGP SIGNATURE-----

--HtyyS77X0bduSMm8--