Subject: Re: Make COPY format extendable: Extract COPY TO format implementations

From: Sutou Kouhei <[email-masked]>
Date: Wed, 24 Jan 2024 14:49:36 +0900 (JST)

Hi,

I've implemented custom COPY format feature based on the
current design discussion. See the attached patches for
details.

I also implemented a PoC COPY format handler for Apache
Arrow with this implementation and it worked.
https://github.com/kou/pg-copy-arrow

The patches implement not only custom COPY TO format feature
but also custom COPY FROM format feature.

0001-0004 is for COPY TO and 0005-0008 is for COPY FROM.

For COPY TO:

0001: This adds CopyToRoutine and use it for text/csv/binary
formats. No implementation change. This just move codes.

0002: This adds support for adding custom COPY TO format by
"CREATE FUNCTION ${FORMAT_NAME}". This uses the same
approach provided by Sawada-san[1] but this doesn't
introduce a wrapper CopyRoutine struct for
Copy{To,From}Routine. Because I noticed that a wrapper
CopyRoutine struct is needless. Copy handler can just return
CopyToRoutine or CopyFromRtouine because both of them have
NodeTag. We can distinct a returned struct by the NodeTag.

[1] https://www.postgresql.org/message-id/[email-masked]

0003: This exports CopyToStateData. No implementation change
except CopyDest enum values. I changed COPY_ prefix to
COPY_DEST_ to avoid name conflict with CopySource enum
values. This just moves codes.

0004: This adds CopyToState::opaque and exports
CopySendEndOfRow(). CopySendEndOfRow() is renamed to
CopyToStateFlush().

For COPY FROM:

0005: Same as 0001 but for COPY FROM. This adds
CopyFromRoutine and use it for text/csv/binary formats. No
implementation change. This just move codes.

0006: Same as 0002 but for COPY FROM. This adds support for
adding custom COPY FROM format by "CREATE FUNCTION
${FORMAT_NAME}".

0007: Same as 0003 but for COPY FROM. This exports
CopyFromStateData. No implementation change except
CopySource enum values. I changed COPY_ prefix to
COPY_SOURCE_ to align CopyDest changes in 0003. This just
moves codes.

0008: Same as 0004 but for COPY FROM. This adds
CopyFromState::opaque and exports
CopyReadBinaryData(). CopyReadBinaryData() is renamed to
CopyFromStateRead().

Thanks,
-- 
kou

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 15 Jan 2024 15:27:02 +0900 (JST),
  Sutou Kouhei <[email-masked]> wrote:

> Hi,
> 
> If there are no more comments for the current design, I'll
> start implementing this feature with the following
> approaches for "Discussing" items:
> 
>> 3.1 Should we use one function(internal) for COPY TO/FROM
>>     handlers or two function(internal)s (one is for COPY TO
>>     handler and another is for COPY FROM handler)?
>>     [4]
> 
> I'll choose "one function(internal) for COPY TO/FROM handlers".
> 
>> 3.4 Should we export Copy{To,From}State? Or should we just
>>     provide getters/setters to access Copy{To,From}State
>>     internal?
>>     [10]
> 
> I'll export Copy{To,From}State.
> 
> 
> Thanks,
> -- 
> kou
> 
> In <[email-masked]>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 12 Jan 2024 14:46:15 +0900 (JST),
>   Sutou Kouhei <[email-masked]> wrote:
> 
>> Hi,
>> 
>> Here is the current summary for a this discussion to make
>> COPY format extendable. It's for reaching consensus and
>> starting implementing the feature. (I'll start implementing
>> the feature once we reach consensus.) If you have any
>> opinion, please share it.
>> 
>> Confirmed:
>> 
>> 1.1 Making COPY format extendable will not reduce performance.
>>     [1]
>> 
>> Decisions:
>> 
>> 2.1 Use separated handler for COPY TO and COPY FROM because
>>     our COPY TO implementation (copyto.c) and COPY FROM
>>     implementation (coypfrom.c) are separated.
>>     [2]
>> 
>> 2.2 Don't use system catalog for COPY TO/FROM handlers. We can
>>     just use a function(internal) that returns a handler instead.
>>     [3]
>> 
>> 2.3 The implementation must include documentation.
>>     [5]
>> 
>> 2.4 The implementation must include test.
>>     [6]
>> 
>> 2.5 The implementation should be consist of small patches
>>     for easy to review.
>>     [6]
>> 
>> 2.7 Copy{To,From}State must have a opaque space for
>>     handlers.
>>     [8]
>> 
>> 2.8 Export CopySendData() and CopySendEndOfRow() for COPY TO
>>     handlers.
>>     [8]
>> 
>> 2.9 Make "format" in PgMsg_CopyOutResponse message
>>     extendable.
>>     [9]
>> 
>> 2.10 Make makeNode() call avoidable in function(internal)
>>      that returns COPY TO/FROM handler.
>>      [9]
>> 
>> 2.11 Custom COPY TO/FROM handlers must be able to parse
>>      their options.
>>      [11]
>> 
>> Discussing:
>> 
>> 3.1 Should we use one function(internal) for COPY TO/FROM
>>     handlers or two function(internal)s (one is for COPY TO
>>     handler and another is for COPY FROM handler)?
>>     [4]
>> 
>> 3.2 If we use separated function(internal) for COPY TO/FROM
>>     handlers, we need to define naming rule. For example,
>>     <method_name>_to(internal) for COPY TO handler and
>>     <method_name>_from(internal) for COPY FROM handler.
>>     [4]
>> 
>> 3.3 Should we use prefix or suffix for function(internal)
>>     name to avoid name conflict with other handlers such as
>>     tablesample handlers?
>>     [7]
>> 
>> 3.4 Should we export Copy{To,From}State? Or should we just
>>     provide getters/setters to access Copy{To,From}State
>>     internal?
>>     [10]
>> 
>> 
>> [1] https://www.postgresql.org/message-id/flat/20231204.153548.2126325458835528809.kou%40clear-code.com
>> [2] https://www.postgresql.org/message-id/flat/ZXEUIy6wl4jHy6Nm%40paquier.xyz
>> [3] https://www.postgresql.org/message-id/flat/CAD21AoAhcZkAp_WDJ4sSv_%2Bg2iCGjfyMFgeu7MxjnjX_FutZAg%40mail.gmail.com
>> [4] https://www.postgresql.org/message-id/flat/CAD21AoDkoGL6yJ_HjNOg9cU%3DaAdW8uQ3rSQOeRS0SX85LPPNwQ%40mail.gmail.com
>> [5] https://www.postgresql.org/message-id/flat/TY3PR01MB9889C9234CD220A3A7075F0DF589A%40TY3PR01MB9889.jpnprd01.prod.outlook.com
>> [6] https://www.postgresql.org/message-id/flat/ZXbiPNriHHyUrcTF%40paquier.xyz
>> [7] https://www.postgresql.org/message-id/flat/20231214.184414.2179134502876898942.kou%40clear-code.com
>> [8] https://www.postgresql.org/message-id/flat/20231221.183504.1240642084042888377.kou%40clear-code.com
>> [9] https://www.postgresql.org/message-id/flat/ZYTfqGppMc9e_w2k%40paquier.xyz
>> [10] https://www.postgresql.org/message-id/flat/CAD21AoD%3DUapH4Wh06G6H5XAzPJ0iJg9YcW8r7E2UEJkZ8QsosA%40mail.gmail.com
>> [11] https://www.postgresql.org/message-id/flat/20240110.152023.1920937326588672387.kou%40clear-code.com
>> 
>> 
>> Thanks,
>> -- 
>> kou
>> 
>> 
> 
>

---

From: Michael Paquier <[email-masked]>
Date: Wed, 24 Jan 2024 17:11:49 +0900

> formats. No implementation change. This just move codes.

10M without this change:

    format,elapsed time (ms)
    text,1090.763
    csv,1136.103
    binary,1137.141

10M with this change:

    format,elapsed time (ms)
    text,1082.654
    csv,1196.991
    binary,1069.697

These numbers point out that binary is faster by 6%, csv is slower by
5%, while text stays around what looks like noise range.  That's not
negligible.  Are these numbers reproducible?  If they are, that could
be a problem for anybody doing bulk-loading of large data sets.  I am
not sure to understand where the improvement for binary comes from by
reading the patch, but perhaps perf would tell more for each format?
The loss with csv could be blamed on the extra manipulations of the
function pointers, likely.
--
Michael

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 24 Jan 2024 07:15:55 -0500

> function pointers, likely.

I don't think that's at all acceptable.

We've spent quite a lot of blood sweat and tears over the years to make 
COPY fast, and we should not sacrifice any of that lightly.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Sutou Kouhei <[email-masked]>
Date: Wed, 24 Jan 2024 23:17:26 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 24 Jan 2024 07:15:55 -0500,
  Andrew Dunstan <[email-masked]> wrote:

> We've spent quite a lot of blood sweat and tears over the years to make COPY
> fast, and we should not sacrifice any of that lightly.

These numbers aren't reproducible. Because these benchmarks
executed on my normal machine not a machine only for
benchmarking. The machine runs another processes such as
editor and Web browser.

For example, here are some results with master
(94edfe250c6a200d2067b0debfe00b4122e9b11e):

Format,N records,Elapsed time (ms)
csv,10000000,1073.715
csv,10000000,1022.830
csv,10000000,1073.584
csv,10000000,1090.651
csv,10000000,1052.259

Here are some results with master + the 0001 patch:

Format,N records,Elapsed time (ms)
csv,10000000,1025.356
csv,10000000,1067.202
csv,10000000,1014.563
csv,10000000,1032.088
csv,10000000,1058.110

I uploaded my benchmark script so that you can run the same
benchmark on your machine:

https://gist.github.com/kou/be02e02e5072c91969469dbf137b5de5

Could anyone try the benchmark with master and master+0001?

Thanks,
-- 
kou

---

From: Sutou Kouhei <[email-masked]>
Date: Wed, 24 Jan 2024 23:20:22 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 24 Jan 2024 14:49:36 +0900 (JST),

> current design discussion. See the attached patches for

I forgot to mention one note. Documentation isn't included
in these patches. I'll write it after all (or some) patches
are merged. Is it OK?

Thanks,
-- 
kou

---

From: jian he <[email-masked]>
Date: Thu, 25 Jan 2024 10:53:58 +0800

> Could anyone try the benchmark with master and master+0001?

sorry. I made a mistake. I applied v6, 0001 to 0008 all the patches.

my tests:
CREATE unlogged TABLE data (a bigint);
SELECT setseed(0.29);
INSERT INTO data SELECT random() * 10000 FROM generate_series(1, 1e7);

my setup:
meson setup --reconfigure ${BUILD} \
-Dprefix=${PG_PREFIX} \
-Dpgport=5462 \
-Dbuildtype=release \
-Ddocs_html_style=website \
-Ddocs_pdf=disabled \
-Dllvm=disabled \
-Dextra_version=_release_build

gcc version:  PostgreSQL 17devel_release_build on x86_64-linux,
compiled by gcc-11.4.0, 64-bit

apply your patch:
COPY data TO '/dev/null' WITH (FORMAT csv) \watch count=5
Time: 668.996 ms
Time: 596.254 ms
Time: 592.723 ms
Time: 591.663 ms
Time: 590.803 ms

not apply your patch, at git 729439607ad210dbb446e31754e8627d7e3f7dda
COPY data TO '/dev/null' WITH (FORMAT csv) \watch count=5
Time: 644.246 ms
Time: 583.075 ms
Time: 568.670 ms
Time: 569.463 ms
Time: 569.201 ms

I forgot to test other formats.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Jan 2024 12:17:55 +0900

>> fast, and we should not sacrifice any of that lightly.

Clearly.

> https://gist.github.com/kou/be02e02e5072c91969469dbf137b5de5

Thanks, that saves time.  I am attaching it to this email as well, for
the sake of the archives if this link is removed in the future.

> Could anyone try the benchmark with master and master+0001?

Yep.  It is one point we need to settle before deciding what to do
with this patch set, and I've done so to reach my own conclusion.

I have a rather good machine at my disposal in the cloud, so I did a
few runs with HEAD and HEAD+0001, with PGDATA mounted on a tmpfs.
Here are some results for the 10M row case, as these should be the
least prone to noise, 5 runs each: 

master
text 10M  1732.570 1684.542 1693.430 1687.696 1714.845
csv 10M   1729.113 1724.926 1727.414 1726.237 1728.865
bin 10M   1679.097 1677.887 1676.764 1677.554 1678.120

master+0001
text 10M  1702.207 1654.818 1647.069 1690.568 1654.446
csv 10M   1764.939 1714.313 1712.444 1712.323 1716.952
bin 10M   1703.061 1702.719 1702.234 1703.346 1704.137

Hmm.  The point of contention in the patch is the change to use the
CopyToOneRow callback in CopyOneRowTo(), as we go through it for each
row and we should habe a worst-case scenario with a relation that has
a small attribute size.  The more rows, the more effect it would have.
The memory context switches and the StringInfo manipulations are
equally important, and there are a bunch of the latter, actually, with
optimizations around fe_msgbuf.

I've repeated a few runs across these two builds, and there is some
variance and noise, but I am going to agree with your point that the
effect 0001 cannot be seen.  Even HEAD is showing some noise.  So I am
discarding the concerns I had after seeing the numbers you posted
upthread.

+typedef bool (*CopyToProcessOption_function) (CopyToState cstate, DefElem *defel);
+typedef int16 (*CopyToGetFormat_function) (CopyToState cstate);
+typedef void (*CopyToStart_function) (CopyToState cstate, TupleDesc tupDesc);
+typedef void (*CopyToOneRow_function) (CopyToState cstate, TupleTableSlot *slot);
+typedef void (*CopyToEnd_function) (CopyToState cstate);

We don't really need a set of typedefs here, let's put the definitions
in the CopyToRoutine struct instead.

+extern CopyToRoutine CopyToRoutineText;
+extern CopyToRoutine CopyToRoutineCSV;
+extern CopyToRoutine CopyToRoutineBinary;

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

Introducing copyapi.h in the initial patch makes sense here for the TO
and FROM routines.

+/* All "text" and "csv" options are parsed in ProcessCopyOptions(). We may
+ * move the code to here later. */
Some areas, like this comment, are written in an incorrect format.

+            if (cstate->opts.csv_mode)
+                CopyAttributeOutCSV(cstate, colname, false,
+                                    list_length(cstate->attnumlist) == 1);
+            else
+                CopyAttributeOutText(cstate, colname);

You are right that this is not worth the trouble of creating a
different set of callbacks for CSV.  This makes the result cleaner.

+    getTypeBinaryOutputInfo(attr->atttypid, &out_func_oid, &isvarlena);
+    fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);

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?  Actually, could it make sense to
split the assignment of cstate->out_functions into its own 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.

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Jan 2024 12:28:01 +0900

> I forgot to test other formats.

There can be some variance in the tests, so you'd better run much more
tests so as you can get a better idea of the mean.  Discarding the N
highest and lowest values also reduces slightly the effects of the
noise you would get across single runs.
--
Michael

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 25 Jan 2024 13:36:03 +0900

> Could anyone try the benchmark with master and master+0001?

I've run a similar scenario:

create unlogged table test (a int);
insert into test select c from generate_series(1, 25000000) c;
copy test to '/tmp/result.csv' with (format csv); -- generates 230MB file

I've run it on HEAD and HEAD+0001 patch and here are the medians of 10
executions for each format:

HEAD:
binary 2930.353 ms
text 2754.852 ms
csv 2890.012 ms

HEAD w/ 0001 patch:
binary 2814.838 ms
text 2900.845 ms
csv 3015.210 ms

Hmm I can see a similar trend that Suto-san had; the binary format got
slightly faster whereas both text and csv format has small regression
(4%~5%). I think that the improvement for binary came from the fact
that we removed "if (cstate->opts.binary)" branches from the original
CopyOneRowTo(). I've experimented with a similar optimization for csv
and text format; have different callbacks for text and csv format and
remove "if (cstate->opts.csv_mode)" branches. I've attached a patch
for that. Here are results:

HEAD w/ 0001 patch + remove branches:
binary 2824.502 ms
text 2715.264 ms
csv 2803.381 ms

The numbers look better now. I'm not sure these are within a noise
range but it might be worth considering having different callbacks for
text and csv formats.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Jan 2024 13:53:30 +0900

> range but it might be worth considering having different callbacks for

Interesting.

Your numbers imply a 0.3% speedup for text, 0.7% speedup for csv and
0.9% speedup for binary, which may be around the noise range assuming
a ~1% range.  While this does not imply a regression, that seems worth
the duplication IMO.  The patch had better document the reason why the
split is done, as well.

CopyFromTextOneRow() has also specific branches for binary and
non-binary removed in 0005, so assuming that I/O is not a bottleneck,
the operation would be faster because we would not evaluate this "if"
condition for each row.  Wouldn't we also see improvements for COPY
FROM with short row values, say when mounting PGDATA into a
tmpfs/ramfs?
--
Michael

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 25 Jan 2024 14:28:38 +0900

On Thu, Jan 25, 2024 at 1:53 PM Michael Paquier <[email-masked]> wrote:
> the duplication IMO.

Agreed. In addition to that, now that each format routine has its own
callbacks, there would be chances that we can do other optimizations
dedicated to the format type in the future if available.

>  The patch had better document the reason why the
> split is done, as well.

+1

> FROM with short row values, say when mounting PGDATA into a

Probably. Seems worth evaluating.

Regards,

---

From: Sutou Kouhei <[email-masked]>
Date: Thu, 25 Jan 2024 17:05:30 +0900 (JST)

Hi,

Thanks for trying these patches!

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 10:53:58 +0800,

> COPY data TO '/dev/null' WITH (FORMAT csv) \watch count=5

Wow! I didn't know the "\watch count="!
I'll use it.

> Time: 590.803 ms

It seems that 5 times isn't enough for this case as Michael
said. But thanks for trying!

Thanks,
-- 
kou

---

From: Sutou Kouhei <[email-masked]>
Date: Thu, 25 Jan 2024 17:45:43 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 12:17:55 +0900,

> in the CopyToRoutine struct instead.

OK. I'll do it.

> parsing the List of options in ProcessCopyOptions().

OK. How about the following for the fetch function
signature?

extern CopyToRoutine *GetBuiltinCopyToRoutine(const char *format);

We may introduce an enum and use it:

typedef enum CopyBuiltinFormat
{
	COPY_BUILTIN_FORMAT_TEXT = 0,
	COPY_BUILTIN_FORMAT_CSV,
	COPY_BUILTIN_FORMAT_BINARY,
} CopyBuiltinFormat;

extern CopyToRoutine *GetBuiltinCopyToRoutine(CopyBuiltinFormat format);

> Some areas, like this comment, are written in an incorrect format.

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

I'll use the following style:

/*
 * ...
 */

> something custom for your own stuff?

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'll chose one of them for a custom type (that isn't
supported by Apache Arrow, e.g. PostGIS types):

1. Report an unsupported error
2. Call output function for Apache Arrow provided by the
   custom type

>                                       Actually, could it make sense to

Yes. Because we need to use getTypeBinaryOutputInfo() for
"binary" and use getTypeOutputInfo() for "text" and "csv".

> implies that as a rule, without a comment that CopyToStart *must* set

CopyToStart doesn't need to set up them if the handler
doesn't use cstate->out_functions.

> independent of the rest.  Then we could move on with 0002~0004 and

OK. I'll focus on 0001 and 0005 for now. I'll restart
0002-0004/0006-0008 after 0001 and 0005 are accepted.

Thanks,
-- 
kou

---

From: Sutou Kouhei <[email-masked]>
Date: Thu, 25 Jan 2024 17:52:55 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 13:36:03 +0900,
  Masahiko Sawada <[email-masked]> wrote:

>                 I've experimented with a similar optimization for csv

Wow! Interesting. I tried the approach before but I didn't
see any difference by the approach. But it may depend on my
environment.

I'll import the approach to the next patch set so that
others can try the approach easily.

Thanks,
-- 
kou

---

From: Michael Paquier <[email-masked]>
Date: Fri, 26 Jan 2024 08:35:19 +0900

> extern CopyToRoutine *GetBuiltinCopyToRoutine(const char *format);

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

> 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

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

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

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

> doesn't use cstate->out_functions.

Noted.

> 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

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 26 Jan 2024 16:18:14 +0800

> others can try the approach easily.

Hi Kou-san,

In the current implementation, there is no way that one can check
incompatibility
options in ProcessCopyOptions, we can postpone the check in CopyFromStart
or CopyToStart, but I think it is a little bit late. Do you think
adding an extra
check for incompatible options hook is acceptable (PFA)?

-- 
Regards
Junwang Zhao

---

From: Sutou Kouhei <[email-masked]>
Date: Fri, 26 Jan 2024 17:32:46 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 16:18:14 +0800,

> check for incompatible options hook is acceptable (PFA)?

Thanks for the suggestion! But I think that a custom handler
can do it in
CopyToProcessOption()/CopyFromProcessOption(). What do you
think about this? Or could you share a sample COPY TO/FROM
WITH() SQL you think?

Thanks,
-- 
kou

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 26 Jan 2024 16:41:50 +0800

> think about this? Or could you share a sample COPY TO/FROM

CopyToProcessOption()/CopyFromProcessOption() can only handle
single option, and store the options in the opaque field,  but it can not
check the relation of two options, for example, considering json format,
the `header` option can not be handled by these two functions.

I want to find a way when the user specifies the header option, customer
handler can error out.

-- 
Regards
Junwang Zhao

---

From: Sutou Kouhei <[email-masked]>
Date: Fri, 26 Jan 2024 17:49:47 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 08:35:19 +0900,

> history with naming things around here.

Thanks for the suggestion.
I rethink about this and use the following:

+extern void ProcessCopyOptionFormatTo(ParseState *pstate, CopyFormatOptions *opts_out, DefElem *defel);

It's not a fetch function. It sets CopyToRoutine opts_out
instead. But it hides CopyToRoutine* to copyto.c. Is it
acceptable?

> https://github.com/michaelpq/pg_plugins/tree/main/blackhole_am

Thanks!

Could you evaluate the attached patch set with COPY FROM?

I attach v7 patch set. It includes only the 0001 and 0005
parts in v6 patch set because we focus on them for now.

0001: This is based on 0001 in v6.

Changes since v6:

* Fix comment style
* Hide CopyToRoutine{Text,CSV,Binary}
* Add more comments
* Eliminate "if (cstate->opts.csv_mode)" branches from "text"
  and "csv" callbacks
* Remove CopyTo*_function typedefs
* Update benchmark results in commit message but the results
  are measured on my environment that isn't suitable for
  accurate benchmark

0002: This is based on 0005 in v6.

Changes since v6:

* Fix comment style
* Hide CopyFromRoutine{Text,CSV,Binary}
* Add more comments
* Eliminate a "if (cstate->opts.csv_mode)" branch from "text"
  and "csv" callbacks
  * NOTE: We can eliminate more "if (cstate->opts.csv_mode)" branches
    such as one in NextCopyFromRawFields(). Should we do it
    in this feature improvement (make COPY format
    extendable)? Can we defer this as a separated improvement?
* Remove CopyFrom*_function typedefs

Thanks,
-- 
kou

---

From: Sutou Kouhei <[email-masked]>
Date: Fri, 26 Jan 2024 17:55:11 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 16:41:50 +0800,

> I want to find a way when the user specifies the header option, customer

Ah, you want to use a built-in option (such as "header")
value from a custom handler, right? Hmm, it may be better
that we call CopyToProcessOption()/CopyFromProcessOption()
for all options including built-in options.

Thanks,
-- 
kou

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 26 Jan 2024 17:02:23 +0800

> for all options including built-in options.
Hmm, still I don't think it can handle all cases, since we don't know
the sequence of the options, we need all the options been parsed
before we check the compatibility of the options, or customer
handlers will need complicated logic to resolve that, which might
lead to ugly code :(

-- 
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Sat, 27 Jan 2024 14:15:02 +0800

Hi Kou-san,

> handlers will need complicated logic to resolve that, which might

I have been working on a *COPY TO JSON* extension since yesterday,
which is based on your V6 patch set, I'd like to give you more input
so you can make better decisions about the implementation(with only
pg-copy-arrow you might not get everything considered).

V8 is based on V6, so anybody involved in the performance issue
should still review the V7 patch set.

0001-0008 is your original V6 implementations

0009 is some changes made by me, I changed CopyToGetFormat to
CopyToSendCopyBegin because pg_copy_json need to send different bytes
in SendCopyBegin, get the format code along is not enough, I once had
a thought that may be we should merge SendCopyBegin/SendCopyEnd into
CopyToStart/CopyToEnd but I don't do that in this patch. I have also
exported more APIs for extension usage.

00010 is the pg_copy_json extension, I think this should be a good
case which can utilize the *extendable copy format* feature, maybe we
should delete copy_test_format if we have this extension as an
example?

-- 
Regards
Junwang Zhao

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 29 Jan 2024 11:41:59 +0900

> handlers will need complicated logic to resolve that, which might

Does it make sense to pass only non-builtin options to the custom
format callback after parsing and evaluating the builtin options? That
is, we parse and evaluate only the builtin options and populate
opts_out first, then pass each rest option to the custom format
handler callback. The callback can refer to the builtin option values.
The callback is expected to return false if the passed option is not
supported. If one of the builtin formats is specified and the rest
options list has at least one option, we raise "option %s not
recognized" error.  IOW it's the core's responsibility to ranse the
"option %s not recognized" error, which is in order to raise a
consistent error message. Also, I think the core should check the
redundant options including bultiin and custom options.

Regards,

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 29 Jan 2024 11:10:45 +0800

> handler callback. The callback can refer to the builtin option values.

Yeah, I think this makes sense.

> redundant options including bultiin and custom options.

It would be good that core could check all the redundant options,
but where should core do the book-keeping of all the options? I have
no idea about this, in my implementation of pg_copy_json extension,
I handle redundant options by adding a xxx_specified field for each
xxx.

-- 
Regards
Junwang Zhao

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 29 Jan 2024 12:21:48 +0900

> I handle redundant options by adding a xxx_specified field for each

What I imagined is that while parsing the all specified options, we
evaluate builtin options and we add non-builtin options to another
list. Then when parsing a non-builtin option, we check if this option
already exists in the list. If there is, we raise the "option %s not
recognized" error.". Once we complete checking all options, we pass
each option in the list to the callback.

Regards,

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 29 Jan 2024 11:37:07 +0800

> each option in the list to the callback.

LGTM.

-- 
Regards
Junwang Zhao

---

From: Sutou Kouhei <[email-masked]>
Date: Mon, 29 Jan 2024 15:03:32 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 27 Jan 2024 14:15:02 +0800,

Thanks!

> in SendCopyBegin, get the format code along is not enough

Oh, I haven't cared about the case.
How about the following API instead?

static void
SendCopyBegin(CopyToState cstate)
{
	StringInfoData buf;

	pq_beginmessage(&buf, PqMsg_CopyOutResponse);
	cstate->opts.to_routine->CopyToFillCopyOutResponse(cstate, &buf);
	pq_endmessage(&buf);
	cstate->copy_dest = COPY_FRONTEND;
}

static void
CopyToJsonFillCopyOutResponse(CopyToState cstate, StringInfoData &buf)
{
	int16		format = 0;

	pq_sendbyte(&buf, format);      /* overall format */
	/*
	 * JSON mode is always one non-binary column
	 */
	pq_sendint16(&buf, 1);
	pq_sendint16(&buf, format);
}

> case which can utilize the *extendable copy format* feature

It seems that it's convenient that we have one more callback
for initializing CopyToState::opaque. It's called only once
when Copy{To,From}Routine is chosen:

typedef struct CopyToRoutine
{
	void		(*CopyToInit) (CopyToState cstate);
...
};

void
ProcessCopyOptions(ParseState *pstate,
				   CopyFormatOptions *opts_out,
				   bool is_from,
				   void *cstate,
				   List *options)
{
...
	foreach(option, options)
	{
		DefElem    *defel = lfirst_node(DefElem, option);

		if (strcmp(defel->defname, "format") == 0)
		{
			...
			opts_out->to_routine = &CopyToRoutineXXX;
			opts_out->to_routine->CopyToInit(cstate);
			...
		}
	}
...
}

>                                                              maybe we

I haven't read the COPY TO format json thread[1] carefully
(sorry), but we may add the JSON format as a built-in
format. If we do it, copy_test_format is useful to test the
extension API.

[1] https://www.postgresql.org/message-id/flat/CALvfUkBxTYy5uWPFVwpk_7ii2zgT07t3d-yR_cy4sfrrLU%3Dkcg%40mail.gmail.com

Thanks,
-- 
kou

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 29 Jan 2024 14:48:40 +0800

>         pq_sendint16(&buf, format);

Make sense to me.

>         void            (*CopyToInit) (CopyToState cstate);

I like this, we can alloc private data in this hook.

> format. If we do it, copy_test_format is useful to test the

Yeah, maybe, I have no strong opinion here, pg_copy_json is
just a toy extension for discussion.

-- 
Regards
Junwang Zhao

---

From: Sutou Kouhei <[email-masked]>
Date: Mon, 29 Jan 2024 18:45:23 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 29 Jan 2024 11:37:07 +0800,

>> each option in the list to the callback.

I implemented this idea and the following ideas:

1. Add init callback for initialization
2. Change GetFormat() to FillCopyXXXResponse()
   because JSON format always use 1 column
3. FROM only: Eliminate more cstate->opts.csv_mode branches
   (This is for performance.)

See the attached v9 patch set for details. Changes since v7:

0001:

* Move CopyToProcessOption() calls to the end of
  ProcessCopyOptions() for easy to option validation
* Add CopyToState::CopyToInit() and call it in
  ProcessCopyOptionFormatTo()
* Change CopyToState::CopyToGetFormat() to
  CopyToState::CopyToFillCopyOutResponse() and use it in
  SendCopyBegin()

0002:

* Move CopyFromProcessOption() calls to the end of
  ProcessCopyOptions() for easy to option validation
* Add CopyFromState::CopyFromInit() and call it in
  ProcessCopyOptionFormatFrom()
* Change CopyFromState::CopyFromGetFormat() to
  CopyFromState::CopyFromFillCopyOutResponse() and use it in
  ReceiveCopyBegin()
* Rename NextCopyFromRawFields() to
  NextCopyFromRawFieldsInternal() and pass the read
  attributes callback explicitly to eliminate more
  cstate->opts.csv_mode branches

Thanks,
-- 
kou

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 30 Jan 2024 11:11:59 +0900

>   CopyToState::CopyToFillCopyOutResponse() and use it in

Thank you for updating the patch! Here are comments on 0001 patch:

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

We need curly brackets for this "if branch" as follows:

if (!format_specifed)
{
    /* Set the default format. */
    ProcessCopyOptionFormatTo(pstate, opts_out, cstate, NULL);
}

---
+        /* Process not built-in options. */
+        foreach(option, unknown_options)
+        {
+                DefElem    *defel = lfirst_node(DefElem, option);
+                bool           processed = false;
+
+                if (!is_from)
+                        processed =
opts_out->to_routine->CopyToProcessOption(cstate, defel);
+                if (!processed)
+                        ereport(ERROR,
+                                        (errcode(ERRCODE_SYNTAX_ERROR),
+                                         errmsg("option \"%s\" not recognized",
+                                                        defel->defname),
+                                         parser_errposition(pstate,
defel->location)));
+        }
+        list_free(unknown_options);

I think we can check the duplicated options in the core as we discussed.

---
+static void
+CopyToTextBasedInit(CopyToState cstate)
+{
+}

and

+static void
+CopyToBinaryInit(CopyToState cstate)
+{
+}

Do we really need separate callbacks for the same behavior? I think we
can have a common init function say CopyToBuitinInit() that does
nothing. Or we can make the init callback optional.

The same is true for process-option callback.

---
         List      *convert_select; /* list of column names (can be NIL) */
+        const          CopyToRoutine *to_routine;      /* callback
routines for COPY TO */
 } CopyFormatOptions;

I think CopyToStateData is a better place to have CopyToRoutine.
copy_data_dest_cb is also there.

---
-                        if (strcmp(fmt, "text") == 0)
-                                 /* default format */ ;
-                        else if (strcmp(fmt, "csv") == 0)
-                                opts_out->csv_mode = true;
-                        else if (strcmp(fmt, "binary") == 0)
-                                opts_out->binary = true;
+
+                        if (is_from)
+                        {
+                                char      *fmt = defGetString(defel);
+
+                                if (strcmp(fmt, "text") == 0)
+                                         /* default format */ ;
+                                else if (strcmp(fmt, "csv") == 0)
+                                {
+                                        opts_out->csv_mode = true;
+                                }
+                                else if (strcmp(fmt, "binary") == 0)
+                                {
+                                        opts_out->binary = true;
+                                }
                         else
-                                ereport(ERROR,
-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                                 errmsg("COPY format
\"%s\" not recognized", fmt\),
-
parser_errposition(pstate, defel->location)));
+                                ProcessCopyOptionFormatTo(pstate,
opts_out, cstate, defel);

The 0002 patch replaces the options checks with
ProcessCopyOptionFormatFrom(). However, both
ProcessCopyOptionFormatTo() and ProcessCOpyOptionFormatFrom() would
set format-related options such as opts_out->csv_mode etc, which seems
not elegant. IIUC the reason why we process only the "format" option
first is to set the callback functions and call the init callback. So
I think we don't necessarily need to do both setting callbacks and
setting format-related options together. Probably we can do only the
callback stuff first and then set format-related options in the
original place we used to do?

---
+static void
+CopyToTextBasedFillCopyOutResponse(CopyToState cstate, StringInfoData *buf)
+{
+        int16          format = 0;
+        int                    natts = list_length(cstate->attnumlist);
+        int                    i;
+
+        pq_sendbyte(buf, format);      /* overall format */
+        pq_sendint16(buf, natts);
+        for (i = 0; i < natts; i++)
+                pq_sendint16(buf, format);     /* per-column formats */
+}

This function and CopyToBinaryFillCopyOutResponse() fill three things:
overall format, the number of columns, and per-column formats. While
this approach is flexible, extensions will have to understand the
format of CopyOutResponse message. An alternative is to have one or
more callbacks that return these three things.

---
+        /* Get info about the columns we need to process. */
+        cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs *
sizeof(Fmgr\Info));
+        foreach(cur, cstate->attnumlist)
+        {
+                int                    attnum = lfirst_int(cur);
+                Oid                    out_func_oid;
+                bool           isvarlena;
+                Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
+
+                getTypeOutputInfo(attr->atttypid, &out_func_oid, &isvarlena);
+                fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+        }

Is preparing the out functions an extension's responsibility? I
thought the core could prepare them based on the overall format
specified by extensions, as long as the overall format matches the
actual data format to send. What do you think?

---
+        /*
+         * Called when COPY TO via the PostgreSQL protocol is
started. This must
+         * fill buf as a valid CopyOutResponse message:
+         *
+         */
+        /*--
+         * +--------+--------+--------+--------+--------+   +--------+--------+
+         * | Format | N attributes    | Attr1's format  |...| AttrN's format  |
+         * +--------+--------+--------+--------+--------+   +--------+--------+
+         * 0: text                      0: text               0: text
+         * 1: binary                    1: binary             1: binary
+         */

I think this kind of diagram could be missed from being updated when
we update the CopyOutResponse format. It's better to refer to the
documentation instead.

Regards,

---

From: Sutou Kouhei <[email-masked]>
Date: Tue, 30 Jan 2024 14:45:31 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 30 Jan 2024 11:11:59 +0900,

> ProcessCopyOptionFormatTo doesn't need to handle NULL case.

Yes, we can do it. But it needs a DefElem allocation. Is it
acceptable?

>     ProcessCopyOptionFormatTo(pstate, opts_out, cstate, NULL);

Oh, sorry. I assumed that pgindent adjusts the style too.

> I think we can check the duplicated options in the core as we discussed.

Oh, sorry. I missed the part. I'll implement it.

> The same is true for process-option callback.

OK. I'll make them optional.

> copy_data_dest_cb is also there.

We can do it but ProcessCopyOptions() accepts NULL
CopyToState for file_fdw. Can we create an empty
CopyToStateData internally like we did for opts_out in
ProcessCopyOptions()? (But it requires exporting
CopyToStateData. We'll export it in a later patch but it's
not yet in 0001.)

> original place we used to do?

If we do it, we need to write the (strcmp(format, "csv") ==
0) condition in copyto.c and copy.c. I wanted to avoid it. I
think that the duplication (setting opts_out->csv_mode in
copyto.c and copyfrom.c) is not a problem. But it's not a
strong opinion. If (strcmp(format, "csv") == 0) duplication
is better than opts_out->csv_mode = true duplication, I'll
do it.

BTW, if we want to make the CSV format implementation more
modularized, we will remove opts_out->csv_mode, move CSV
related options to CopyToCSVProcessOption() and keep CSV
related options in its opaque space. For example,
opts_out->force_quote exists in COPY TO opaque space but
doesn't exist in COPY FROM opaque space because it's not
used in COPY FROM.

> more callbacks that return these three things.

Yes, we can choose the approach. I don't have a strong
opinion on which approach to choose.

> actual data format to send. What do you think?

Hmm. I want to keep the preparation as an extension's
responsibility. Because it's not needed for all formats. For
example, Apache Arrow FORMAT doesn't need it. And JSON
FORMAT doesn't need it too because it use
composite_to_json().

> +         * fill buf as a valid CopyOutResponse message:

It makes sense. I couldn't find the documentation when I
wrote it but I found it now...:
https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY

Is there recommended comment style to refer a documentation?
"See doc/src/sgml/protocol.sgml for the CopyOutResponse
message details" is OK?

Thanks,
-- 
kou

---

From: Michael Paquier <[email-masked]>
Date: Tue, 30 Jan 2024 16:20:54 +0900

> Yes, we can do it. But it needs a DefElem allocation. Is it

I don't think that there is a need for a DelElem at all here?  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.

>> nothing. Or we can make the init callback optional.

Keeping empty options does not strike as a bad idea, because this
forces extension developers to think about this code path rather than
just ignore it.  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.

> FORMAT doesn't need it too because it use

I agree that it could be really useful for extensions to be able to
force that.  We already know that for the in-core formats we've cared
about being able to enforce the way data is handled in input and
output.

> "See doc/src/sgml/protocol.sgml for the CopyOutResponse

There are a couple of places in the C code where we refer to SGML docs
when it comes to specific details, so using a method like that here to
avoid a duplication with the docs sounds sensible for me.

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?
--
Michael

---

From: Sutou Kouhei <[email-masked]>
Date: Tue, 30 Jan 2024 17:15:11 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 30 Jan 2024 16:20:54 +0900,
  Michael Paquier <[email-masked]> wrote:

> 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

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

>                  Now, all the Init() callbacks are empty for the

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

> independent on the extensibility.  Sutou-san, would you be OK with

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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 30 Jan 2024 17:37:35 +0900

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

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

> not sure how much performance will improve by this but it

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 
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 :)

> (+ reverting opts_out->{csv_mode,binary} changes in

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

---

From: Sutou Kouhei <[email-masked]>
Date: Wed, 31 Jan 2024 14:11:22 +0900 (JST)

Hi,

In <[email-masked]>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 30 Jan 2024 17:37:35 +0900,

> the callback routines in opts_out if the same validation happens a few

Ah, yes. defel->location is used in later patches. For
example, it's used when a COPY handler for the specified
FORMAT isn't found.

>                           I am really worried about the complexities

I understand your concern. Let's introduce minimal callbacks
as the first step. I think that we completed our design
discussion for this feature. We can choose minimal callbacks
based on the discussion.

>         The custom options don't seem like an absolute strong

Interesting. Let's remove custom options support from the
initial minimal callbacks.

> that would help quite a bit.  :)

I've prepared the v10 patch set. Could you try this?

Changes since the v7 patch set:

0001:

* Remove CopyToProcessOption() callback
* Remove CopyToGetFormat() callback
* Revert passing CopyToState to ProcessCopyOptions()
* Revert moving "opts_out->{csv_mode,binary} = true" to
  ProcessCopyOptionFormatTo()
* Change to receive "const char *format" instead "DefElem  *defel"
  by ProcessCopyOptionFormatTo()

0002:

* Remove CopyFromProcessOption() callback
* Remove CopyFromGetFormat() callback
* Change to receive "const char *format" instead "DefElem
  *defel" by ProcessCopyOptionFormatFrom()
* Remove "if (cstate->opts.csv_mode)" branches from
  NextCopyFromRawFields()

FYI: Here are Copy{From,To}Routine in the v10 patch set. I
think that only Copy{From,To}OneRow are minimal callbacks
for the performance gain. But can we keep Copy{From,To}Start
and Copy{From,To}End for consistency? We can remove a few
{csv_mode,binary} conditions by Copy{From,To}{Start,End}. It
doesn't depend on the number of COPY target tuples. So they
will not affect performance.

/* Routines for a COPY FROM format implementation. */
typedef struct CopyFromRoutine
{
	/*
	 * Called when COPY FROM is started. This will initialize something and
	 * receive a header.
	 */
	void		(*CopyFromStart) (CopyFromState cstate, TupleDesc tupDesc);

	/* Copy one row. It returns false if no more tuples. */
	bool		(*CopyFromOneRow) (CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls);

	/* Called when COPY FROM is ended. This will finalize something. */
	void		(*CopyFromEnd) (CopyFromState cstate);
}			CopyFromRoutine;

/* Routines for a COPY TO format implementation. */
typedef struct CopyToRoutine
{
	/* Called when COPY TO is started. This will send a header. */
	void		(*CopyToStart) (CopyToState cstate, TupleDesc tupDesc);

	/* Copy one row for COPY TO. */
	void		(*CopyToOneRow) (CopyToState cstate, TupleTableSlot *slot);

	/* Called when COPY TO is ended. This will send a trailer. */
	void		(*CopyToEnd) (CopyToState cstate);
}			CopyToRoutine;

Thanks,
-- 
kou

---

From: Michael Paquier <[email-masked]>
Date: Wed, 31 Jan 2024 14:39:54 +0900

> example, it's used when a COPY handler for the specified

I see.

> I've prepared the v10 patch set. Could you try this?

Thanks, I'm looking into that now.

> will not affect performance.

I think I'm OK to keep the start/end callbacks.  This makes the code
more consistent as a whole, as well.
--
Michael