Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 26 Jan 2024 08:35:19 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sutou Kouhei <kou@clear-code.com>
Cc: andrew@dunslane.net, sawada.mshk@gmail.com, zhjwpku@gmail.com,
	nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
Message-ID: <ZbLwNyOKbddno0Ue@paquier.xyz>
References: <10025bac-158c-ffe7-fbec-32b42629121f@dunslane.net>
 <20240124.231726.1771099323950062661.kou@clear-code.com>
 <ZbHS439y-Bs6HIAR@paquier.xyz>
 <20240125.174543.1042528588176841299.kou@clear-code.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="vrDO5TzygvVLxf2c"
Content-Disposition: inline
In-Reply-To: <20240125.174543.1042528588176841299.kou@clear-code.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZbLwNyOKbddno0Ue%40paquier.xyz>
Precedence: bulk


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

On Thu, Jan 25, 2024 at 05:45:43PM +0900, Sutou Kouhei wrote:
> In <ZbHS439y-Bs6HIAR@paquier.xyz>
>   "Re: Make COPY format extendable: Extract COPY TO format implementation=
s" on Thu, 25 Jan 2024 12:17:55 +0900,
>   Michael Paquier <michael@paquier.xyz> wrote:
>> +extern CopyToRoutine CopyToRoutineText;
>> +extern CopyToRoutine CopyToRoutineCSV;
>> +extern CopyToRoutine CopyToRoutineBinary;
>>=20
>> All that should IMO remain in copyto.c and copyfrom.c in the initial
>> patch doing the refactoring.  Why not using a fetch function instead
>> that uses a string in input?  Then you can call that once after
>> parsing the List of options in ProcessCopyOptions().
>=20
> OK. How about the following for the fetch function
> signature?
>=20
> extern CopyToRoutine *GetBuiltinCopyToRoutine(const char *format);

Or CopyToRoutineGet()?  I am not wedded to my suggestion, got a bad
history with naming things around here.

> We may introduce an enum and use it:
>=20
> typedef enum CopyBuiltinFormat
> {
> 	COPY_BUILTIN_FORMAT_TEXT =3D 0,
> 	COPY_BUILTIN_FORMAT_CSV,
> 	COPY_BUILTIN_FORMAT_BINARY,
> } CopyBuiltinFormat;
>=20
> extern CopyToRoutine *GetBuiltinCopyToRoutine(CopyBuiltinFormat format);

I am not sure that this is necessary as the option value is a string.

> Oh, sorry. I assumed that the comment style was adjusted by
> pgindent.

No worries, that's just something we get used to.  I tend to fix a lot
of these things by myself when editing patches.

>> +    getTypeBinaryOutputInfo(attr->atttypid, &out_func_oid, &isvarlena);
>> +    fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
>>=20
>> Actually, this split is interesting.  It is possible for a custom
>> format to plug in a custom set of out functions.  Did you make use of
>> something custom for your own stuff?
>=20
> I didn't. My PoC custom COPY format handler for Apache Arrow
> just handles integer and text for now. It doesn't use
> cstate->out_functions because cstate->out_functions may not
> return a valid binary format value for Apache Arrow. So it
> formats each value by itself.

I mean, if you use a custom output function, you could tweak things
even more with byteas or such..  If a callback is expected to do
something, like setting the output function OIDs in the start
callback, we'd better document it rather than letting that be implied.

>>                                       Actually, could it make sense to
>> split the assignment of cstate->out_functions into its own callback?
>=20
> Yes. Because we need to use getTypeBinaryOutputInfo() for
> "binary" and use getTypeOutputInfo() for "text" and "csv".

Okay.  After sleeping on it, a split makes sense here, because it also
reduces the presence of TupleDesc in the start callback.

>> Sure, that's part of the start phase, but at least it would make clear
>> that a custom method *has* to assign these OIDs to work.  The patch
>> implies that as a rule, without a comment that CopyToStart *must* set
>> up these OIDs.
>=20
> CopyToStart doesn't need to set up them if the handler
> doesn't use cstate->out_functions.

Noted.

>> I think that 0001 and 0005 should be handled first, as pieces
>> independent of the rest.  Then we could move on with 0002~0004 and
>> 0006~0008.
>=20
> OK. I'll focus on 0001 and 0005 for now. I'll restart
> 0002-0004/0006-0008 after 0001 and 0005 are accepted.

Once you get these, I'd be interested in re-doing an evaluation of
COPY TO and more tests with COPY FROM while running Postgres on
scissors.  One thing I was thinking to use here is my blackhole_am for
COPY FROM:
https://github.com/michaelpq/pg_plugins/tree/main/blackhole_am

As per its name, it does nothing on INSERT, so you could create a
table using it as access method, and stress the COPY FROM execution
paths without having to mount Postgres on a tmpfs because the data is
sent to the void.  Perhaps it does not matter, but that moves the
tests to the bottlenecks we want to stress (aka the per-row callback
for large data sets).

I've switched the patch as waiting on author for now.  Thanks for your
perseverance here.  I understand that's not easy to follow up with
patches and reviews (^_^;)
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWy8DcACgkQnvQgOdby
QH1GKg//SBDwJ8voWKSQK0qJ4bJHs4CrRrv+d+kqejTCdApoN7YpNS1AdwMGbV+h
6sdYrC+aBVFTV2UArEIhYtMzz5Ex+/bUQMDsdC3oLVv29VO2sGw6OGxShfPfb7mc
9NLGsMGFknw41I09hQV4q6HPPzKQ2cGxDGZ+4AQEi9SzrT42ZGnwWrh4WXiFvCHE
+C3HHnrx1qs5TpJgM9CSLmxwhEUnH4PJju0+j1JeKRvnRrgX2E2pzBFQuLA5ahW7
jgRvODL6doOVNwNeS8vdkghvb1iqrHogwlZYtRNiKodyFFc4WswZ4jPE6omSdXPX
Xft38C3Dj+mRWNy28hpr1SIaxOw/VwOeSX0iyJ14NPys3kyaJ5h98uWlF40L8Lzb
SjhF6TjY1W1PuthWnOJVaNW/iD2Ep0tCd73+gcrP9o+f0ePnJq/S29B9UgSj5Ydb
WGKTQpyelNTeDy219HS/G5H2VGbnyXMp1MURU6O1xXCCJCQCsXRy8rKPASVg/bp2
HLqxgacYaWCAIrlnEFexUZx3sKasamVCD3BYIhea7QyrTdG6ElQvjkGN1ny5eqTf
i4qhSesMULQPsqNQgjUuPikzineTdaWx+K7T6uUCiygMiPUhfGCkk6QE2+q4czdq
AG32Xja8m3/l0QChfYGJjKyrXj6tBYqeIEVjr4phS1GL9lIXdWU=
=2wYf
-----END PGP SIGNATURE-----

--vrDO5TzygvVLxf2c--