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

Hi,

I want to work on making COPY format extendable. I attach
the first patch for it. I'll send more patches after this is
merged.

Background:

Currently, COPY TO/FROM supports only "text", "csv" and
"binary" formats. There are some requests to support more
COPY formats. For example:

* 2023-11: JSON and JSON lines [1]
* 2022-04: Apache Arrow [2]
* 2018-02: Apache Avro, Apache Parquet and Apache ORC [3]

(FYI: I want to add support for Apache Arrow.)

There were discussions how to add support for more formats. [3][4]
In these discussions, we got a consensus about making COPY
format extendable.

But it seems that nobody works on this yet. So I want to
work on this. (If there is anyone who wants to work on this
together, I'm happy.)

Summary:

The attached patch introduces CopyToFormatOps struct that is
similar to TupleTableSlotOps for TupleTableSlot but
CopyToFormatOps is for COPY TO format. CopyToFormatOps has
routines to implement a COPY TO format.

The attached patch doesn't change:

* the current behavior (all existing tests are still passed
without changing them)
* the existing "text", "csv" and "binary" format output
implementations including local variable names (the
attached patch just move them and adjust indent)
* performance (no significant loss of performance)

In other words, this is just a refactoring for further
changes to make COPY format extendable. If I use "complete
the task and then request reviews for it" approach, it will
be difficult to review because changes for it will be
large. So I want to work on this step by step. Is it
acceptable?

TODOs that should be done in subsequent patches:

* Add some CopyToState readers such as CopyToStateGetDest(),
CopyToStateGetAttnums() and CopyToStateGetOpts()
(We will need to consider which APIs should be exported.)
(This is for implemeing COPY TO format by extension.)
* Export CopySend*() in src/backend/commands/copyto.c
(This is for implemeing COPY TO format by extension.)
* Add API to register a new COPY TO format implementation
* Add "CREATE XXX" to register a new COPY TO format (or COPY
TO/FROM format) implementation
("CREATE COPY HANDLER" was suggested in [5].)
* Same for COPY FROM

Performance:

We got a consensus about making COPY format extendable but
we should care about performance. [6]

> I think that step 1 ought to be to convert the existing
> formats into plug-ins, and demonstrate that there's no
> significant loss of performance.

So I measured COPY TO time with/without this change. You can
see there is no significant loss of performance.

Data: Random 32 bit integers:

CREATE TABLE data (int32 integer);
INSERT INTO data
SELECT random() * 10000
FROM generate_series(1, ${n_records});

The number of records: 100K, 1M and 10M

100K without this change:

format,elapsed time (ms)
text,22.527
csv,23.822
binary,24.806

100K with this change:

format,elapsed time (ms)
text,22.919
csv,24.643
binary,24.705

1M without this change:

format,elapsed time (ms)
text,223.457
csv,233.583
binary,242.687

1M with this change:

format,elapsed time (ms)
text,224.591
csv,233.964
binary,247.164

10M without this change:

format,elapsed time (ms)
text,2330.383
csv,2411.394
binary,2590.817

10M with this change:

format,elapsed time (ms)
text,2231.307
csv,2408.067
binary,2473.617

[1]: https://www.postgresql.org/message-id/flat/24e3ee88-ec1e-421b-89ae-8a47ee0d2df1%40joeconway.com#a5e6b8829f9a74dfc835f6f29f2e44c5
[2]: https://www.postgresql.org/message-id/flat/CAGrfaBVyfm0wPzXVqm0%3Dh5uArYh9N_ij%2BsVpUtDHqkB%3DVyB3jw%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/flat/20180210151304.fonjztsynewldfba%40gmail.com
[4]: https://www.postgresql.org/message-id/flat/3741749.1655952719%40sss.pgh.pa.us#2bb7af4a3d2c7669f9a49808d777a20d
[5]: https://www.postgresql.org/message-id/20180211211235.5x3jywe5z3lkgcsr%40alap3.anarazel.de
[6]: https://www.postgresql.org/message-id/3741749.1655952719%40sss.pgh.pa.us

Thanks,
--
kou

On Mon, Dec 04, 2023 at 03:35:48PM +0900, Sutou Kouhei wrote:
> I want to work on making COPY format extendable. I attach
> the first patch for it. I'll send more patches after this is
> merged.

Given the current discussion about adding JSON, I think this could be a
nice bit of refactoring that could ultimately open the door to providing
other COPY formats via shared libraries.

> In other words, this is just a refactoring for further
> changes to make COPY format extendable. If I use "complete
> the task and then request reviews for it" approach, it will
> be difficult to review because changes for it will be
> large. So I want to work on this step by step. Is it
> acceptable?

I think it makes sense to do this part independently, but we should be
careful to design this with the follow-up tasks in mind.

> So I measured COPY TO time with/without this change. You can
> see there is no significant loss of performance.
>
> Data: Random 32 bit integers:
>
> CREATE TABLE data (int32 integer);
> INSERT INTO data
> SELECT random() * 10000
> FROM generate_series(1, ${n_records});

Seems encouraging. I assume the performance concerns stem from the use of
function pointers. Or was there something else?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi,

Thanks for replying to this proposal!

In <20231205182458(dot)GC2757816(at)nathanxps13>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 5 Dec 2023 12:24:58 -0600,
Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:

> I think it makes sense to do this part independently, but we should be
> careful to design this with the follow-up tasks in mind.

OK. I'll keep updating the "TODOs" section in the original
e-mail. It also includes design in the follow-up tasks. We
can discuss the design separately from the patches
submitting. (The current submitted patch just focuses on
refactoring but we can discuss the final design.)

> I assume the performance concerns stem from the use of
> function pointers. Or was there something else?

I think so too.

The original e-mail that mentioned the performance concern
[1] didn't say about the reason but the use of function
pointers might be concerned.

If the currently supported formats ("text", "csv" and
"binary") are implemented as an extension, it may have more
concerns but we will keep them as built-in formats for
compatibility. So I think that no more concerns exist for
these formats.

[1]: https://www.postgresql.org/message-id/flat/3741749.1655952719%40sss.pgh.pa.us#2bb7af4a3d2c7669f9a49808d777a20d

Thanks,
--
kou

On Wed, Dec 6, 2023 at 10:45 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> Thanks for replying to this proposal!
>
> In <20231205182458(dot)GC2757816(at)nathanxps13>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 5 Dec 2023 12:24:58 -0600,
> Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> > I think it makes sense to do this part independently, but we should be
> > careful to design this with the follow-up tasks in mind.
>
> OK. I'll keep updating the "TODOs" section in the original
> e-mail. It also includes design in the follow-up tasks. We
> can discuss the design separately from the patches
> submitting. (The current submitted patch just focuses on
> refactoring but we can discuss the final design.)
>
> > I assume the performance concerns stem from the use of
> > function pointers. Or was there something else?
>
> I think so too.
>
> The original e-mail that mentioned the performance concern
> [1] didn't say about the reason but the use of function
> pointers might be concerned.
>
> If the currently supported formats ("text", "csv" and
> "binary") are implemented as an extension, it may have more
> concerns but we will keep them as built-in formats for
> compatibility. So I think that no more concerns exist for
> these formats.
>

For the modern formats(parquet, orc, avro, etc.), will they be
implemented as extensions or in core?

The patch looks good except for a pair of extra curly braces.

>
> [1]: https://www.postgresql.org/message-id/flat/3741749.1655952719%40sss.pgh.pa.us#2bb7af4a3d2c7669f9a49808d777a20d
>
>
> Thanks,
> --
> kou
>
>

--
Regards
Junwang Zhao

Hi,

In <CAEG8a3Jf7kPV3ez5OHu-pFGscKfVyd9KkubMF199etkfz=EPRg(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 6 Dec 2023 11:18:35 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

> For the modern formats(parquet, orc, avro, etc.), will they be
> implemented as extensions or in core?

I think that they should be implemented as extensions
because they will depend of external libraries and may not
use C. For example, C++ will be used for Apache Parquet
because the official Apache Parquet C++ implementation
exists but the C implementation doesn't.

(I can implement an extension for Apache Parquet after we
complete this feature. I'll implement an extension for
Apache Arrow with the official Apache Arrow C++
implementation. And it's easy that we convert Apache Arrow
data to Apache Parquet with the official Apache Parquet
implementation.)

> The patch looks good except for a pair of extra curly braces.

Thanks for the review! I attach the v2 patch that removes
extra curly braces for "if (isnull)".

Thanks,
--
kou

On Wed, Dec 6, 2023 at 2:19 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3Jf7kPV3ez5OHu-pFGscKfVyd9KkubMF199etkfz=EPRg(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 6 Dec 2023 11:18:35 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> > For the modern formats(parquet, orc, avro, etc.), will they be
> > implemented as extensions or in core?
>
> I think that they should be implemented as extensions
> because they will depend of external libraries and may not
> use C. For example, C++ will be used for Apache Parquet
> because the official Apache Parquet C++ implementation
> exists but the C implementation doesn't.
>
> (I can implement an extension for Apache Parquet after we
> complete this feature. I'll implement an extension for
> Apache Arrow with the official Apache Arrow C++
> implementation. And it's easy that we convert Apache Arrow
> data to Apache Parquet with the official Apache Parquet
> implementation.)
>
> > The patch looks good except for a pair of extra curly braces.
>
> Thanks for the review! I attach the v2 patch that removes
> extra curly braces for "if (isnull)".
>
For the extra curly braces, I mean the following code block in
CopyToFormatBinaryStart:

+ { <-- I thought this is useless?
+ /* Generate header for a binary copy */
+ int32 tmp;
+
+ /* Signature */
+ CopySendData(cstate, BinarySignature, 11);
+ /* Flags field */
+ tmp = 0;
+ CopySendInt32(cstate, tmp);
+ /* No header extension */
+ tmp = 0;
+ CopySendInt32(cstate, tmp);
+ }

>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

Hi,

In <CAEG8a3K9dE2gt3+K+h=DwTqMenR84aeYuYS+cty3SR3LAeDBAQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 6 Dec 2023 15:11:34 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

> For the extra curly braces, I mean the following code block in
> CopyToFormatBinaryStart:
>
> + { <-- I thought this is useless?
> + /* Generate header for a binary copy */
> + int32 tmp;
> +
> + /* Signature */
> + CopySendData(cstate, BinarySignature, 11);
> + /* Flags field */
> + tmp = 0;
> + CopySendInt32(cstate, tmp);
> + /* No header extension */
> + tmp = 0;
> + CopySendInt32(cstate, tmp);
> + }

Oh, I see. I've removed and attach the v3 patch. In general,
I don't change variable name and so on in this patch. I just
move codes in this patch. But I also removed the "tmp"
variable for this case because I think that the name isn't
suitable for larger scope. (I think that "tmp" is acceptable
in a small scope like the above code.)

New code:

/* Generate header for a binary copy */
/* Signature */
CopySendData(cstate, BinarySignature, 11);
/* Flags field */
CopySendInt32(cstate, 0);
/* No header extension */
CopySendInt32(cstate, 0);

Thanks,
--
kou

Sutou Kouhei wrote:

> * 2022-04: Apache Arrow [2]
> * 2018-02: Apache Avro, Apache Parquet and Apache ORC [3]
>
> (FYI: I want to add support for Apache Arrow.)
>
> There were discussions how to add support for more formats. [3][4]
> In these discussions, we got a consensus about making COPY
> format extendable.

These formats seem all column-oriented whereas COPY is row-oriented
at the protocol level [1].
With regard to the procotol, how would it work to support these formats?

[1] https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY

Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Wed, Dec 6, 2023 at 3:28 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3K9dE2gt3+K+h=DwTqMenR84aeYuYS+cty3SR3LAeDBAQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 6 Dec 2023 15:11:34 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> > For the extra curly braces, I mean the following code block in
> > CopyToFormatBinaryStart:
> >
> > + { <-- I thought this is useless?
> > + /* Generate header for a binary copy */
> > + int32 tmp;
> > +
> > + /* Signature */
> > + CopySendData(cstate, BinarySignature, 11);
> > + /* Flags field */
> > + tmp = 0;
> > + CopySendInt32(cstate, tmp);
> > + /* No header extension */
> > + tmp = 0;
> > + CopySendInt32(cstate, tmp);
> > + }
>
> Oh, I see. I've removed and attach the v3 patch. In general,
> I don't change variable name and so on in this patch. I just
> move codes in this patch. But I also removed the "tmp"
> variable for this case because I think that the name isn't
> suitable for larger scope. (I think that "tmp" is acceptable
> in a small scope like the above code.)
>
> New code:
>
> /* Generate header for a binary copy */
> /* Signature */
> CopySendData(cstate, BinarySignature, 11);
> /* Flags field */
> CopySendInt32(cstate, 0);
> /* No header extension */
> CopySendInt32(cstate, 0);
>
>
> Thanks,
> --
> kou

Hi Kou,

I read the thread[1] you posted and I think Andres's suggestion sounds great.

Should we extract both *copy to* and *copy from* for the first step, in that
case we can add the pg_copy_handler catalog smoothly later.

Attached V4 adds 'extract copy from' and it passed the cirrus ci,
please take a look.

I added a hook *copy_from_end* but this might be removed later if not used.

[1]: https://www.postgresql.org/message-id/20180211211235.5x3jywe5z3lkgcsr%40alap3.anarazel.de
--
Regards
Junwang Zhao

On Wed, Dec 6, 2023 at 8:32 PM Daniel Verite <daniel(at)manitou-mail(dot)org> wrote:
>
> Sutou Kouhei wrote:
>
> > * 2022-04: Apache Arrow [2]
> > * 2018-02: Apache Avro, Apache Parquet and Apache ORC [3]
> >
> > (FYI: I want to add support for Apache Arrow.)
> >
> > There were discussions how to add support for more formats. [3][4]
> > In these discussions, we got a consensus about making COPY
> > format extendable.
>
>
> These formats seem all column-oriented whereas COPY is row-oriented
> at the protocol level [1].
> With regard to the procotol, how would it work to support these formats?
>

They have kind of *RowGroup* concepts, a bunch of rows goes to a RowBatch
and the data of the same column goes together.

I think they should fit the COPY semantics and there are some FDW out there for
these modern formats, like [1]. If we support COPY to deal with the
format, it will
be easier to interact with them(without creating
server/usermapping/foreign table).

[1]: https://github.com/adjust/parquet_fdw

>
> [1] https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY
>
>
> Best regards,
> --
> Daniel Vérité
> https://postgresql.verite.pro/
> Twitter: @DanielVerite
>
>

--
Regards
Junwang Zhao

On Wed, Dec 06, 2023 at 10:07:51PM +0800, Junwang Zhao wrote:
> I read the thread[1] you posted and I think Andres's suggestion sounds great.
>
> Should we extract both *copy to* and *copy from* for the first step, in that
> case we can add the pg_copy_handler catalog smoothly later.
>
> Attached V4 adds 'extract copy from' and it passed the cirrus ci,
> please take a look.
>
> I added a hook *copy_from_end* but this might be removed later if not used.
>
> [1]: https://www.postgresql.org/message-id/20180211211235.5x3jywe5z3lkgcsr%40alap3.anarazel.de

I was looking at the differences between v3 posted by Sutou-san and
v4 from you, seeing that:

+/* Routines for a COPY HANDLER implementation. */
+typedef struct CopyHandlerOps
{
/* Called when COPY TO is started. This will send a header. */
- void (*start) (CopyToState cstate, TupleDesc tupDesc);
+ void (*copy_to_start) (CopyToState cstate, TupleDesc tupDesc);

/* Copy one row for COPY TO. */
- void (*one_row) (CopyToState cstate, TupleTableSlot *slot);
+ void (*copy_to_one_row) (CopyToState cstate, TupleTableSlot *slot);

/* Called when COPY TO is ended. This will send a trailer. */
- void (*end) (CopyToState cstate);
-} CopyToFormatOps;
+ void (*copy_to_end) (CopyToState cstate);
+
+ void (*copy_from_start) (CopyFromState cstate, TupleDesc tupDesc);
+ bool (*copy_from_next) (CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls);
+ void (*copy_from_error_callback) (CopyFromState cstate);
+ void (*copy_from_end) (CopyFromState cstate);
+} CopyHandlerOps;

And we've spent a good deal of time refactoring the copy code so as
the logic behind TO and FROM is split. Having a set of routines that
groups both does not look like a step in the right direction to me,
and v4 is an attempt at solving two problems, while v3 aims to improve
one case. It seems to me that each callback portion should be focused
on staying in its own area of the code, aka copyfrom*.c or copyto*.c.
--
Michael

Hi,

In <CAEG8a3LSRhK601Bn50u71BgfNWm4q3kv-o-KEq=hrbyLbY_EsA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 6 Dec 2023 22:07:51 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

> Should we extract both *copy to* and *copy from* for the first step, in that
> case we can add the pg_copy_handler catalog smoothly later.

I don't object it (mixing TO/FROM changes to one patch) but
it may make review difficult. Is it acceptable?

FYI: I planed that I implement TO part, and then FROM part,
and then unify TO/FROM parts if needed. [1]

> Attached V4 adds 'extract copy from' and it passed the cirrus ci,
> please take a look.

Thanks. Here are my comments:

> + /*
> + * Error is relevant to a particular line.
> + *
> + * If line_buf still contains the correct line, print it.
> + */
> + if (cstate->line_buf_valid)

We need to fix the indentation.

> +CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc)
> +{
> + FmgrInfo *in_functions;
> + Oid *typioparams;
> + Oid in_func_oid;
> + AttrNumber num_phys_attrs;
> +
> + /*
> + * Pick up the required catalog information for each attribute in the
> + * relation, including the input function, the element type (to pass to
> + * the input function), and info about defaults and constraints. (Which
> + * input function we use depends on text/binary format choice.)
> + */
> + num_phys_attrs = tupDesc->natts;
> + in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
> + typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));

We need to update the comment because defaults and
constraints aren't picked up here.

> +CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc)
...
> + /*
> + * Pick up the required catalog information for each attribute in the
> + * relation, including the input function, the element type (to pass to
> + * the input function), and info about defaults and constraints. (Which
> + * input function we use depends on text/binary format choice.)
> + */
> + in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
> + typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));

ditto.

> @@ -1716,15 +1776,6 @@ BeginCopyFrom(ParseState *pstate,
> ReceiveCopyBinaryHeader(cstate);
> }

I think that this block should be moved to
CopyFromFormatBinaryStart() too. But we need to run it after
we setup inputs such as data_source_cb, pipe and filename...

+/* Routines for a COPY HANDLER implementation. */
+typedef struct CopyHandlerOps
+{
+ /* Called when COPY TO is started. This will send a header. */
+ void (*copy_to_start) (CopyToState cstate, TupleDesc tupDesc);
+
+ /* Copy one row for COPY TO. */
+ void (*copy_to_one_row) (CopyToState cstate, TupleTableSlot *slot);
+
+ /* Called when COPY TO is ended. This will send a trailer. */
+ void (*copy_to_end) (CopyToState cstate);
+
+ void (*copy_from_start) (CopyFromState cstate, TupleDesc tupDesc);
+ bool (*copy_from_next) (CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls);
+ void (*copy_from_error_callback) (CopyFromState cstate);
+ void (*copy_from_end) (CopyFromState cstate);
+} CopyHandlerOps;

It seems that "copy_" prefix is redundant. Should we use
"to_start" instead of "copy_to_start" and so on?

BTW, it seems that "COPY FROM (FORMAT json)" may not be implemented. [2]
We may need to care about NULL copy_from_* cases.

> I added a hook *copy_from_end* but this might be removed later if not used.

It may be useful to clean up resources for COPY FROM but the
patch doesn't call the copy_from_end. How about removing it
for now? We can add it and call it from EndCopyFrom() later?
Because it's not needed for now.

I think that we should focus on refactoring instead of
adding a new feature in this patch.

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

Thanks,
--
kou

On Thu, Dec 7, 2023 at 8:39 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Dec 06, 2023 at 10:07:51PM +0800, Junwang Zhao wrote:
> > I read the thread[1] you posted and I think Andres's suggestion sounds great.
> >
> > Should we extract both *copy to* and *copy from* for the first step, in that
> > case we can add the pg_copy_handler catalog smoothly later.
> >
> > Attached V4 adds 'extract copy from' and it passed the cirrus ci,
> > please take a look.
> >
> > I added a hook *copy_from_end* but this might be removed later if not used.
> >
> > [1]: https://www.postgresql.org/message-id/20180211211235.5x3jywe5z3lkgcsr%40alap3.anarazel.de
>
> I was looking at the differences between v3 posted by Sutou-san and
> v4 from you, seeing that:
>
> +/* Routines for a COPY HANDLER implementation. */
> +typedef struct CopyHandlerOps
> {
> /* Called when COPY TO is started. This will send a header. */
> - void (*start) (CopyToState cstate, TupleDesc tupDesc);
> + void (*copy_to_start) (CopyToState cstate, TupleDesc tupDesc);
>
> /* Copy one row for COPY TO. */
> - void (*one_row) (CopyToState cstate, TupleTableSlot *slot);
> + void (*copy_to_one_row) (CopyToState cstate, TupleTableSlot *slot);
>
> /* Called when COPY TO is ended. This will send a trailer. */
> - void (*end) (CopyToState cstate);
> -} CopyToFormatOps;
> + void (*copy_to_end) (CopyToState cstate);
> +
> + void (*copy_from_start) (CopyFromState cstate, TupleDesc tupDesc);
> + bool (*copy_from_next) (CopyFromState cstate, ExprContext *econtext,
> + Datum *values, bool *nulls);
> + void (*copy_from_error_callback) (CopyFromState cstate);
> + void (*copy_from_end) (CopyFromState cstate);
> +} CopyHandlerOps;
>
> And we've spent a good deal of time refactoring the copy code so as
> the logic behind TO and FROM is split. Having a set of routines that
> groups both does not look like a step in the right direction to me,

The point of this refactor (from my view) is to make it possible to add new
copy handlers in extensions, just like access method. As Andres suggested,
a system catalog like *pg_copy_handler*, if we split TO and FROM into two
sets of routines, does that mean we have to create two catalog(
pg_copy_from_handler and pg_copy_to_handler)?

> and v4 is an attempt at solving two problems, while v3 aims to improve
> one case. It seems to me that each callback portion should be focused
> on staying in its own area of the code, aka copyfrom*.c or copyto*.c.
> --
> Michael

--
Regards
Junwang Zhao

On Thu, Dec 7, 2023 at 1:05 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3LSRhK601Bn50u71BgfNWm4q3kv-o-KEq=hrbyLbY_EsA(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 6 Dec 2023 22:07:51 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> > Should we extract both *copy to* and *copy from* for the first step, in that
> > case we can add the pg_copy_handler catalog smoothly later.
>
> I don't object it (mixing TO/FROM changes to one patch) but
> it may make review difficult. Is it acceptable?
>
> FYI: I planed that I implement TO part, and then FROM part,
> and then unify TO/FROM parts if needed. [1]

I'm fine with step by step refactoring, let's just wait for more
suggestions.

>
> > Attached V4 adds 'extract copy from' and it passed the cirrus ci,
> > please take a look.
>
> Thanks. Here are my comments:
>
> > + /*
> > + * Error is relevant to a particular line.
> > + *
> > + * If line_buf still contains the correct line, print it.
> > + */
> > + if (cstate->line_buf_valid)
>
> We need to fix the indentation.
>
> > +CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc)
> > +{
> > + FmgrInfo *in_functions;
> > + Oid *typioparams;
> > + Oid in_func_oid;
> > + AttrNumber num_phys_attrs;
> > +
> > + /*
> > + * Pick up the required catalog information for each attribute in the
> > + * relation, including the input function, the element type (to pass to
> > + * the input function), and info about defaults and constraints. (Which
> > + * input function we use depends on text/binary format choice.)
> > + */
> > + num_phys_attrs = tupDesc->natts;
> > + in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
> > + typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
>
> We need to update the comment because defaults and
> constraints aren't picked up here.
>
> > +CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc)
> ...
> > + /*
> > + * Pick up the required catalog information for each attribute in the
> > + * relation, including the input function, the element type (to pass to
> > + * the input function), and info about defaults and constraints. (Which
> > + * input function we use depends on text/binary format choice.)
> > + */
> > + in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
> > + typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
>
> ditto.
>
>
> > @@ -1716,15 +1776,6 @@ BeginCopyFrom(ParseState *pstate,
> > ReceiveCopyBinaryHeader(cstate);
> > }
>
> I think that this block should be moved to
> CopyFromFormatBinaryStart() too. But we need to run it after
> we setup inputs such as data_source_cb, pipe and filename...
>
> +/* Routines for a COPY HANDLER implementation. */
> +typedef struct CopyHandlerOps
> +{
> + /* Called when COPY TO is started. This will send a header. */
> + void (*copy_to_start) (CopyToState cstate, TupleDesc tupDesc);
> +
> + /* Copy one row for COPY TO. */
> + void (*copy_to_one_row) (CopyToState cstate, TupleTableSlot *slot);
> +
> + /* Called when COPY TO is ended. This will send a trailer. */
> + void (*copy_to_end) (CopyToState cstate);
> +
> + void (*copy_from_start) (CopyFromState cstate, TupleDesc tupDesc);
> + bool (*copy_from_next) (CopyFromState cstate, ExprContext *econtext,
> + Datum *values, bool *nulls);
> + void (*copy_from_error_callback) (CopyFromState cstate);
> + void (*copy_from_end) (CopyFromState cstate);
> +} CopyHandlerOps;
>
> It seems that "copy_" prefix is redundant. Should we use
> "to_start" instead of "copy_to_start" and so on?
>
> BTW, it seems that "COPY FROM (FORMAT json)" may not be implemented. [2]
> We may need to care about NULL copy_from_* cases.
>
>
> > I added a hook *copy_from_end* but this might be removed later if not used.
>
> It may be useful to clean up resources for COPY FROM but the
> patch doesn't call the copy_from_end. How about removing it
> for now? We can add it and call it from EndCopyFrom() later?
> Because it's not needed for now.
>
> I think that we should focus on refactoring instead of
> adding a new feature in this patch.
>
>
> [1]: https://www.postgresql.org/message-id/20231204.153548.2126325458835528809.kou%40clear-code.com
> [2]: https://www.postgresql.org/message-id/flat/CALvfUkBxTYy5uWPFVwpk_7ii2zgT07t3d-yR_cy4sfrrLU%3Dkcg%40mail.gmail.com
>
>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

On 2023-12-07 Th 03:37, Junwang Zhao wrote:
>
> The point of this refactor (from my view) is to make it possible to add new
> copy handlers in extensions, just like access method. As Andres suggested,
> a system catalog like *pg_copy_handler*, if we split TO and FROM into two
> sets of routines, does that mean we have to create two catalog(
> pg_copy_from_handler and pg_copy_to_handler)?

Surely not. Either have two fields, one for the TO handler and one for
the FROM handler, or a flag on each row indicating if it's a FROM or TO
handler.

cheers

andrew

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

On Fri, Dec 8, 2023 at 1:39 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> On 2023-12-07 Th 03:37, Junwang Zhao wrote:
> >
> > The point of this refactor (from my view) is to make it possible to add new
> > copy handlers in extensions, just like access method. As Andres suggested,
> > a system catalog like *pg_copy_handler*, if we split TO and FROM into two
> > sets of routines, does that mean we have to create two catalog(
> > pg_copy_from_handler and pg_copy_to_handler)?
>
>
>
> Surely not. Either have two fields, one for the TO handler and one for
> the FROM handler, or a flag on each row indicating if it's a FROM or TO
> handler.

True.

But why do we need a system catalog like pg_copy_handler in the first
place? I imagined that an extension can define a handler function
returning a set of callbacks and the parser can lookup the handler
function by name, like FDW and TABLESAMPLE.

Regards,

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

On Fri, Dec 8, 2023 at 3:27 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Fri, Dec 8, 2023 at 1:39 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> >
> >
> > On 2023-12-07 Th 03:37, Junwang Zhao wrote:
> > >
> > > The point of this refactor (from my view) is to make it possible to add new
> > > copy handlers in extensions, just like access method. As Andres suggested,
> > > a system catalog like *pg_copy_handler*, if we split TO and FROM into two
> > > sets of routines, does that mean we have to create two catalog(
> > > pg_copy_from_handler and pg_copy_to_handler)?
> >
> >
> >
> > Surely not. Either have two fields, one for the TO handler and one for
> > the FROM handler, or a flag on each row indicating if it's a FROM or TO
> > handler.

If we wrap the two fields into a single structure, that will still be in
copy.h, which I think is not necessary. A single routing wrapper should
be enough, the actual implementation still stays separate
copy_[to/from].c files.

>
> True.
>
> But why do we need a system catalog like pg_copy_handler in the first
> place? I imagined that an extension can define a handler function
> returning a set of callbacks and the parser can lookup the handler
> function by name, like FDW and TABLESAMPLE.
>
I can see FDW related utility commands but no TABLESAMPLE related,
and there is a pg_foreign_data_wrapper system catalog which has
a *fdwhandler* field.

If we want extensions to create a new copy handler, I think
something like pg_copy_hander should be necessary.

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

I go one step further to implement the pg_copy_handler, attached V5 is
the implementation with some changes suggested by Kou.

You can also review this on this github pull request [1].

[1]: https://github.com/zhjwpku/postgres/pull/1/files

--
Regards
Junwang Zhao

On Fri, Dec 08, 2023 at 10:32:27AM +0800, Junwang Zhao wrote:
> I can see FDW related utility commands but no TABLESAMPLE related,
> and there is a pg_foreign_data_wrapper system catalog which has
> a *fdwhandler* field.

+ */ +CATALOG(pg_copy_handler,4551,CopyHandlerRelationId)

Using a catalog is an over-engineered design. Others have provided
hints about that upthread, but it would be enough to have one or two
handler types that are wrapped around one or two SQL *functions*, like
tablesamples. It seems like you've missed it, but feel free to read
about tablesample-method.sgml, that explains how this is achieved for
tablesamples.

> If we want extensions to create a new copy handler, I think
> something like pg_copy_hander should be necessary.

A catalog is not necessary, that's the point, because it can be
replaced by a scan of pg_proc with the function name defined in a COPY
query (be it through a FORMAT, or different option in a DefElem).
An example of extension with tablesamples is contrib/tsm_system_rows/,
that just uses a function returning a tsm_handler:
CREATE FUNCTION system_rows(internal)
RETURNS tsm_handler
AS 'MODULE_PATHNAME', 'tsm_system_rows_handler'
LANGUAGE C STRICT;

Then SELECT queries rely on the contents of the TABLESAMPLE clause to
find the set of callbacks it should use by calling the function.

+/* Routines for a COPY HANDLER implementation. */
+typedef struct CopyRoutine
+{

FWIW, I find weird the concept of having one handler for both COPY
FROM and COPY TO as each one of them has callbacks that are mutually
exclusive to the other, but I'm OK if there is a consensus of only
one. So I'd suggest to use *two* NodeTags instead for a cleaner
split, meaning that we'd need two functions for each method. My point
is that a custom COPY handler could just define a COPY TO handler or a
COPY FROM handler, though it mostly comes down to a matter of taste
regarding how clean the error handling becomes if one tries to use a
set of callbacks with a COPY type (TO or FROM) not matching it.
--
Michael

On Fri, Dec 8, 2023 at 2:17 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Dec 08, 2023 at 10:32:27AM +0800, Junwang Zhao wrote:
> > I can see FDW related utility commands but no TABLESAMPLE related,
> > and there is a pg_foreign_data_wrapper system catalog which has
> > a *fdwhandler* field.
>
> + */ +CATALOG(pg_copy_handler,4551,CopyHandlerRelationId)
>
> Using a catalog is an over-engineered design. Others have provided
> hints about that upthread, but it would be enough to have one or two
> handler types that are wrapped around one or two SQL *functions*, like
> tablesamples. It seems like you've missed it, but feel free to read
> about tablesample-method.sgml, that explains how this is achieved for
> tablesamples.

Agreed. My previous example of FDW was not a good one, I missed something.

>
> > If we want extensions to create a new copy handler, I think
> > something like pg_copy_hander should be necessary.
>
> A catalog is not necessary, that's the point, because it can be
> replaced by a scan of pg_proc with the function name defined in a COPY
> query (be it through a FORMAT, or different option in a DefElem).
> An example of extension with tablesamples is contrib/tsm_system_rows/,
> that just uses a function returning a tsm_handler:
> CREATE FUNCTION system_rows(internal)
> RETURNS tsm_handler
> AS 'MODULE_PATHNAME', 'tsm_system_rows_handler'
> LANGUAGE C STRICT;
>
> Then SELECT queries rely on the contents of the TABLESAMPLE clause to
> find the set of callbacks it should use by calling the function.
>
> +/* Routines for a COPY HANDLER implementation. */
> +typedef struct CopyRoutine
> +{
>
> FWIW, I find weird the concept of having one handler for both COPY
> FROM and COPY TO as each one of them has callbacks that are mutually
> exclusive to the other, but I'm OK if there is a consensus of only
> one. So I'd suggest to use *two* NodeTags instead for a cleaner
> split, meaning that we'd need two functions for each method. My point
> is that a custom COPY handler could just define a COPY TO handler or a
> COPY FROM handler, though it mostly comes down to a matter of taste
> regarding how clean the error handling becomes if one tries to use a
> set of callbacks with a COPY type (TO or FROM) not matching it.

I tend to agree to have separate two functions for each method. But
given we implement it in tablesample-way, I think we need to make it
clear how to call one of the two functions depending on COPY TO and
FROM.

IIUC in tablesamples cases, we scan pg_proc to find the handler
function like system_rows(internal) by the method name specified in
the query. On the other hand, in COPY cases, the queries would be
going to be like:

COPY tab TO stdout WITH (format = 'arrow');
and
COPY tab FROM stdin WITH (format = 'arrow');

So a custom COPY extension would not be able to define SQL functions
just like arrow(internal) for example. We might need to define a rule
like the function returning copy_in/out_handler must be defined as
<method name>_to(internal) and <method_name>_from(internal).

Regards,

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

On Fri, Dec 08, 2023 at 03:42:06PM +0900, Masahiko Sawada wrote:
> So a custom COPY extension would not be able to define SQL functions
> just like arrow(internal) for example. We might need to define a rule
> like the function returning copy_in/out_handler must be defined as
> <method name>_to(internal) and <method_name>_from(internal).

Yeah, I was wondering if there was a trick to avoid the input internal
argument conflict, but cannot recall something elegant on the top of
my mind. Anyway, I'd be OK with any approach as long as it plays
nicely with the query integration, and that's FORMAT's DefElem with
its string value to do the function lookups.
--
Michael

Dear Junagn, Sutou-san,

Basically I agree your point - improving a extendibility is good.
(I remember that this theme was talked at Japan PostgreSQL conference)
Below are my comments for your patch.

01. General

Just to confirm - is it OK to partially implement APIs? E.g., only COPY TO is
available. Currently it seems not to consider a case which is not implemented.

02. General

It might be trivial, but could you please clarify how users can extend? Is it OK
to do below steps?

1. Create a handler function, via CREATE FUNCTION,
2. Register a handler, via new SQL (CREATE COPY HANDLER),
3. Specify the added handler as COPY ... FORMAT clause.

03. General

Could you please add document-related tasks to your TODO? I imagined like
fdwhandler.sgml.

04. General - copyright

For newly added files, the below copyright seems sufficient. See applyparallelworker.c.

```
* Copyright (c) 2023, PostgreSQL Global Development Group
```

05. src/include/catalog/* files

IIUC, 8000 or higher OIDs should be used while developing a patch. src/include/catalog/unused_oids
would suggest a candidate which you can use.

06. copy.c

I felt that we can create files per copying methods, like copy_{text|csv|binary}.c,
like indexes.
How do other think?

07. fmt_to_name()

I'm not sure the function is really needed. Can we follow like get_foreign_data_wrapper_oid()
and remove the funciton?

08. GetCopyRoutineByName()

Should we use syscache for searching a catalog?

09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()

Comments still refer CopyHandlerOps, whereas it was renamed.

10. copy.h

Per foreign.h and fdwapi.h, should we add a new header file and move some APIs?

11. copy.h

```
-/* These are private in commands/copy[from|to].c */
-typedef struct CopyFromStateData *CopyFromState;
-typedef struct CopyToStateData *CopyToState;
```

Are above changes really needed?

12. CopyFormatOptions

Can we remove `bool binary` in future?

13. external functions

```
+extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
+extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot *slot);
+extern void CopyToFormatTextEnd(CopyToState cstate);
+extern void CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc);
+extern bool CopyFromFormatTextNext(CopyFromState cstate, ExprContext *econtext,
+
Datum *values, bool *nulls);
+extern void CopyFromFormatTextErrorCallback(CopyFromState cstate);
+
+extern void CopyToFormatBinaryStart(CopyToState cstate, TupleDesc tupDesc);
+extern void CopyToFormatBinaryOneRow(CopyToState cstate, TupleTableSlot *slot);
+extern void CopyToFormatBinaryEnd(CopyToState cstate);
+extern void CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc);
+extern bool CopyFromFormatBinaryNext(CopyFromState cstate,
ExprContext *econtext,
+
Datum *values, bool *nulls);
+extern void CopyFromFormatBinaryErrorCallback(CopyFromState cstate);
```

FYI - If you add files for {text|csv|binary}, these declarations can be removed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Sat, Dec 9, 2023 at 10:43 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Junagn, Sutou-san,
>
> Basically I agree your point - improving a extendibility is good.
> (I remember that this theme was talked at Japan PostgreSQL conference)
> Below are my comments for your patch.
>
> 01. General
>
> Just to confirm - is it OK to partially implement APIs? E.g., only COPY TO is
> available. Currently it seems not to consider a case which is not implemented.
>
For partially implements, we can leave the hook as NULL, and check the NULL
at *ProcessCopyOptions* and report error if not supported.

> 02. General
>
> It might be trivial, but could you please clarify how users can extend? Is it OK
> to do below steps?
>
> 1. Create a handler function, via CREATE FUNCTION,
> 2. Register a handler, via new SQL (CREATE COPY HANDLER),
> 3. Specify the added handler as COPY ... FORMAT clause.
>
My original thought was option 2, but as Michael point, option 1 is
the right way
to go.

> 03. General
>
> Could you please add document-related tasks to your TODO? I imagined like
> fdwhandler.sgml.
>
> 04. General - copyright
>
> For newly added files, the below copyright seems sufficient. See applyparallelworker.c.
>
> ```
> * Copyright (c) 2023, PostgreSQL Global Development Group
> ```
>
> 05. src/include/catalog/* files
>
> IIUC, 8000 or higher OIDs should be used while developing a patch. src/include/catalog/unused_oids
> would suggest a candidate which you can use.

Yeah, I will run renumber_oids.pl at last.

>
> 06. copy.c
>
> I felt that we can create files per copying methods, like copy_{text|csv|binary}.c,
> like indexes.
> How do other think?

Not sure about this, it seems others have put a lot of effort into
splitting TO and From.
Also like to hear from others.

>
> 07. fmt_to_name()
>
> I'm not sure the function is really needed. Can we follow like get_foreign_data_wrapper_oid()
> and remove the funciton?

I have referenced some code from greenplum, will remove this.

>
> 08. GetCopyRoutineByName()
>
> Should we use syscache for searching a catalog?
>
> 09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()
>
> Comments still refer CopyHandlerOps, whereas it was renamed.
>
> 10. copy.h
>
> Per foreign.h and fdwapi.h, should we add a new header file and move some APIs?
>
> 11. copy.h
>
> ```
> -/* These are private in commands/copy[from|to].c */
> -typedef struct CopyFromStateData *CopyFromState;
> -typedef struct CopyToStateData *CopyToState;
> ```
>
> Are above changes really needed?
>
> 12. CopyFormatOptions
>
> Can we remove `bool binary` in future?
>
> 13. external functions
>
> ```
> +extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
> +extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot *slot);
> +extern void CopyToFormatTextEnd(CopyToState cstate);
> +extern void CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc);
> +extern bool CopyFromFormatTextNext(CopyFromState cstate, ExprContext *econtext,
> +
> Datum *values, bool *nulls);
> +extern void CopyFromFormatTextErrorCallback(CopyFromState cstate);
> +
> +extern void CopyToFormatBinaryStart(CopyToState cstate, TupleDesc tupDesc);
> +extern void CopyToFormatBinaryOneRow(CopyToState cstate, TupleTableSlot *slot);
> +extern void CopyToFormatBinaryEnd(CopyToState cstate);
> +extern void CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc);
> +extern bool CopyFromFormatBinaryNext(CopyFromState cstate,
> ExprContext *econtext,
> +
> Datum *values, bool *nulls);
> +extern void CopyFromFormatBinaryErrorCallback(CopyFromState cstate);
> ```
>
> FYI - If you add files for {text|csv|binary}, these declarations can be removed.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>

Thanks for all the valuable suggestions.

--
Regards
Junwang Zhao

Hi Junwang

Please also see my presentation slides from last years PostgreSQL
Conference in Berlin (attached)

The main Idea is to make not just "format", but also "transport" and
"stream processing" extendable via virtual function tables.

Btw, will any of you here be in Prague next week ?
Would be a good opportunity to discuss this in person.

Best Regards
Hannu

On Sat, Dec 9, 2023 at 9:39 AM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> On Sat, Dec 9, 2023 at 10:43 AM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Dear Junagn, Sutou-san,
> >
> > Basically I agree your point - improving a extendibility is good.
> > (I remember that this theme was talked at Japan PostgreSQL conference)
> > Below are my comments for your patch.
> >
> > 01. General
> >
> > Just to confirm - is it OK to partially implement APIs? E.g., only COPY TO is
> > available. Currently it seems not to consider a case which is not implemented.
> >
> For partially implements, we can leave the hook as NULL, and check the NULL
> at *ProcessCopyOptions* and report error if not supported.
>
> > 02. General
> >
> > It might be trivial, but could you please clarify how users can extend? Is it OK
> > to do below steps?
> >
> > 1. Create a handler function, via CREATE FUNCTION,
> > 2. Register a handler, via new SQL (CREATE COPY HANDLER),
> > 3. Specify the added handler as COPY ... FORMAT clause.
> >
> My original thought was option 2, but as Michael point, option 1 is
> the right way
> to go.
>
> > 03. General
> >
> > Could you please add document-related tasks to your TODO? I imagined like
> > fdwhandler.sgml.
> >
> > 04. General - copyright
> >
> > For newly added files, the below copyright seems sufficient. See applyparallelworker.c.
> >
> > ```
> > * Copyright (c) 2023, PostgreSQL Global Development Group
> > ```
> >
> > 05. src/include/catalog/* files
> >
> > IIUC, 8000 or higher OIDs should be used while developing a patch. src/include/catalog/unused_oids
> > would suggest a candidate which you can use.
>
> Yeah, I will run renumber_oids.pl at last.
>
> >
> > 06. copy.c
> >
> > I felt that we can create files per copying methods, like copy_{text|csv|binary}.c,
> > like indexes.
> > How do other think?
>
> Not sure about this, it seems others have put a lot of effort into
> splitting TO and From.
> Also like to hear from others.
>
> >
> > 07. fmt_to_name()
> >
> > I'm not sure the function is really needed. Can we follow like get_foreign_data_wrapper_oid()
> > and remove the funciton?
>
> I have referenced some code from greenplum, will remove this.
>
> >
> > 08. GetCopyRoutineByName()
> >
> > Should we use syscache for searching a catalog?
> >
> > 09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()
> >
> > Comments still refer CopyHandlerOps, whereas it was renamed.
> >
> > 10. copy.h
> >
> > Per foreign.h and fdwapi.h, should we add a new header file and move some APIs?
> >
> > 11. copy.h
> >
> > ```
> > -/* These are private in commands/copy[from|to].c */
> > -typedef struct CopyFromStateData *CopyFromState;
> > -typedef struct CopyToStateData *CopyToState;
> > ```
> >
> > Are above changes really needed?
> >
> > 12. CopyFormatOptions
> >
> > Can we remove `bool binary` in future?
> >
> > 13. external functions
> >
> > ```
> > +extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
> > +extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot *slot);
> > +extern void CopyToFormatTextEnd(CopyToState cstate);
> > +extern void CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc);
> > +extern bool CopyFromFormatTextNext(CopyFromState cstate, ExprContext *econtext,
> > +
> > Datum *values, bool *nulls);
> > +extern void CopyFromFormatTextErrorCallback(CopyFromState cstate);
> > +
> > +extern void CopyToFormatBinaryStart(CopyToState cstate, TupleDesc tupDesc);
> > +extern void CopyToFormatBinaryOneRow(CopyToState cstate, TupleTableSlot *slot);
> > +extern void CopyToFormatBinaryEnd(CopyToState cstate);
> > +extern void CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc);
> > +extern bool CopyFromFormatBinaryNext(CopyFromState cstate,
> > ExprContext *econtext,
> > +
> > Datum *values, bool *nulls);
> > +extern void CopyFromFormatBinaryErrorCallback(CopyFromState cstate);
> > ```
> >
> > FYI - If you add files for {text|csv|binary}, these declarations can be removed.
> >
> > Best Regards,
> > Hayato Kuroda
> > FUJITSU LIMITED
> >
>
> Thanks for all the valuable suggestions.
>
> --
> Regards
> Junwang Zhao
>
>

Hi,

Thanks for reviewing our latest patch!

In
<TY3PR01MB9889C9234CD220A3A7075F0DF589A(at)TY3PR01MB9889(dot)jpnprd01(dot)prod(dot)outlook(dot)com>
"RE: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 9 Dec 2023 02:43:49 +0000,
"Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:

> (I remember that this theme was talked at Japan PostgreSQL conference)

Yes. I should have talked to you more at the conference...
I will do it next time!

Can we discuss how to proceed this improvement?

There are 2 approaches for it:

1. Do the followings concurrently:
a. Implementing small changes that got a consensus and
merge them step-by-step
(e.g. We got a consensus that we need to extract the
current format related routines.)
b. Discuss design

(v1-v3 patches use this approach.)

2. Implement one (large) complete patch set with design
discussion and merge it

(v4- patches use this approach.)

Which approach is preferred? (Or should we choose another
approach?)

I thought that 1. is preferred because it will reduce review
cost. So I chose 1.

If 2. is preferred, I'll use 2. (I'll add more changes to
the latest patch.)

Thanks,
--
kou

Hi,

In <CAD21AoDkoGL6yJ_HjNOg9cU=aAdW8uQ3rSQOeRS0SX85LPPNwQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 8 Dec 2023 15:42:06 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> So a custom COPY extension would not be able to define SQL functions
> just like arrow(internal) for example. We might need to define a rule
> like the function returning copy_in/out_handler must be defined as
> <method name>_to(internal) and <method_name>_from(internal).

We may not need to add "_to"/"_from" suffix by checking both
of argument type and return type. Because we use different
return type for copy_in/out_handler.

But the current LookupFuncName() family doesn't check return
type. If we use this approach, we need to improve the
current LookupFuncName() family too.

Thanks,
--
kou

Hi,

In <CAMT0RQRqVo4fGDWHqOn+wr_eoiXQVfyC=8-c=H=y6VcNxi6BvQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 9 Dec 2023 12:38:46 +0100,
Hannu Krosing <hannuk(at)google(dot)com> wrote:

> Please also see my presentation slides from last years PostgreSQL
> Conference in Berlin (attached)

Thanks for sharing your idea here.

> The main Idea is to make not just "format", but also "transport" and
> "stream processing" extendable via virtual function tables.

"Transport" and "stream processing" are out of scope in this
thread. How about starting new threads for them and discuss
them there?

> Btw, will any of you here be in Prague next week ?

Sorry. No.

Thanks,
--
kou

On Sun, Dec 10, 2023 at 4:44 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> Thanks for reviewing our latest patch!
>
> In
> <TY3PR01MB9889C9234CD220A3A7075F0DF589A(at)TY3PR01MB9889(dot)jpnprd01(dot)prod(dot)outlook(dot)com>
> "RE: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 9 Dec 2023 02:43:49 +0000,
> "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> > (I remember that this theme was talked at Japan PostgreSQL conference)
>
> Yes. I should have talked to you more at the conference...
> I will do it next time!
>
>
> Can we discuss how to proceed this improvement?
>
> There are 2 approaches for it:
>
> 1. Do the followings concurrently:
> a. Implementing small changes that got a consensus and
> merge them step-by-step
> (e.g. We got a consensus that we need to extract the
> current format related routines.)
> b. Discuss design
>
> (v1-v3 patches use this approach.)
>
> 2. Implement one (large) complete patch set with design
> discussion and merge it
>
> (v4- patches use this approach.)
>
> Which approach is preferred? (Or should we choose another
> approach?)
>
> I thought that 1. is preferred because it will reduce review
> cost. So I chose 1.
>
> If 2. is preferred, I'll use 2. (I'll add more changes to
> the latest patch.)
>
I'm ok with both, and I'd like to work with you for the parquet
extension, excited about this new feature, thanks for bringing
this up.

Forgive me for making so much noise about approach 2, I
just want to hear about more suggestions of the final shape
of this feature.

>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

On Sun, Dec 10, 2023 at 5:44 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> Thanks for reviewing our latest patch!
>
> In
> <TY3PR01MB9889C9234CD220A3A7075F0DF589A(at)TY3PR01MB9889(dot)jpnprd01(dot)prod(dot)outlook(dot)com>
> "RE: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 9 Dec 2023 02:43:49 +0000,
> "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> > (I remember that this theme was talked at Japan PostgreSQL conference)
>
> Yes. I should have talked to you more at the conference...
> I will do it next time!
>
>
> Can we discuss how to proceed this improvement?
>
> There are 2 approaches for it:
>
> 1. Do the followings concurrently:
> a. Implementing small changes that got a consensus and
> merge them step-by-step
> (e.g. We got a consensus that we need to extract the
> current format related routines.)
> b. Discuss design

It's preferable to make patches small for easy review. We can merge
them anytime before commit if necessary.

I think we need to discuss overall design about callbacks and how
extensions define a custom copy handler etc. It may require some PoC
patches. Once we have a consensus on overall design we polish patches
including the documentation changes and regression tests.

Regards,

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

On Sun, Dec 10, 2023 at 5:55 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoDkoGL6yJ_HjNOg9cU=aAdW8uQ3rSQOeRS0SX85LPPNwQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 8 Dec 2023 15:42:06 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > So a custom COPY extension would not be able to define SQL functions
> > just like arrow(internal) for example. We might need to define a rule
> > like the function returning copy_in/out_handler must be defined as
> > <method name>_to(internal) and <method_name>_from(internal).
>
> We may not need to add "_to"/"_from" suffix by checking both
> of argument type and return type. Because we use different
> return type for copy_in/out_handler.
>
> But the current LookupFuncName() family doesn't check return
> type. If we use this approach, we need to improve the
> current LookupFuncName() family too.

IIUC we cannot create two same name functions with the same arguments
but a different return value type in the first place. It seems to me
to be an overkill to change such a design.

Another idea is to encapsulate copy_to/from_handler by a super class
like copy_handler. The handler function is called with an argument,
say copyto, and returns copy_handler encapsulating either
copy_to/from_handler depending on the argument.

Regards,

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

On Mon, Dec 11, 2023 at 10:57:15AM +0900, Masahiko Sawada wrote:
> IIUC we cannot create two same name functions with the same arguments
> but a different return value type in the first place. It seems to me
> to be an overkill to change such a design.

Agreed to not touch the logictics of LookupFuncName() for the sake of
this thread. I have not checked the SQL specification, but I recall
that there are a few assumptions from the spec embedded in the lookup
logic particularly when it comes to specify a procedure name without
arguments.

> Another idea is to encapsulate copy_to/from_handler by a super class
> like copy_handler. The handler function is called with an argument,
> say copyto, and returns copy_handler encapsulating either
> copy_to/from_handler depending on the argument.

Yep, that's possible as well and can work as a cross-check between the
argument and the NodeTag assigned to the handler structure returned by
the function.

At the end, the final result of the patch should IMO include:
- Documentation about how one can register a custom copy_handler.
- Something in src/test/modules/, minimalistic still useful that can
be used as a template when one wants to implement their own handler.
The documentation should mention about this module.
- No need for SQL functions for all the in-core handlers: let's just
return pointers to them based on the options given.

It would be probably cleaner to split the patch so as the code is
refactored and evaluated with the in-core handlers first, and then
extended with the pluggable facilities and the function lookups.
--
Michael

On Sat, Dec 9, 2023 at 7:38 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
>
> Hi Junwang
>
> Please also see my presentation slides from last years PostgreSQL
> Conference in Berlin (attached)

I read through the slides, really promising ideas, it's will be great
if we can get there at last.

>
> The main Idea is to make not just "format", but also "transport" and
> "stream processing" extendable via virtual function tables.
The code is really coupled, it is not easy to do all of these in one round,
it will be great if you have a POC patch.

>
> Btw, will any of you here be in Prague next week ?
> Would be a good opportunity to discuss this in person.
Sorry, no.

>
>
> Best Regards
> Hannu
>
> On Sat, Dec 9, 2023 at 9:39 AM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > On Sat, Dec 9, 2023 at 10:43 AM Hayato Kuroda (Fujitsu)
> > <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> > >
> > > Dear Junagn, Sutou-san,
> > >
> > > Basically I agree your point - improving a extendibility is good.
> > > (I remember that this theme was talked at Japan PostgreSQL conference)
> > > Below are my comments for your patch.
> > >
> > > 01. General
> > >
> > > Just to confirm - is it OK to partially implement APIs? E.g., only COPY TO is
> > > available. Currently it seems not to consider a case which is not implemented.
> > >
> > For partially implements, we can leave the hook as NULL, and check the NULL
> > at *ProcessCopyOptions* and report error if not supported.
> >
> > > 02. General
> > >
> > > It might be trivial, but could you please clarify how users can extend? Is it OK
> > > to do below steps?
> > >
> > > 1. Create a handler function, via CREATE FUNCTION,
> > > 2. Register a handler, via new SQL (CREATE COPY HANDLER),
> > > 3. Specify the added handler as COPY ... FORMAT clause.
> > >
> > My original thought was option 2, but as Michael point, option 1 is
> > the right way
> > to go.
> >
> > > 03. General
> > >
> > > Could you please add document-related tasks to your TODO? I imagined like
> > > fdwhandler.sgml.
> > >
> > > 04. General - copyright
> > >
> > > For newly added files, the below copyright seems sufficient. See applyparallelworker.c.
> > >
> > > ```
> > > * Copyright (c) 2023, PostgreSQL Global Development Group
> > > ```
> > >
> > > 05. src/include/catalog/* files
> > >
> > > IIUC, 8000 or higher OIDs should be used while developing a patch. src/include/catalog/unused_oids
> > > would suggest a candidate which you can use.
> >
> > Yeah, I will run renumber_oids.pl at last.
> >
> > >
> > > 06. copy.c
> > >
> > > I felt that we can create files per copying methods, like copy_{text|csv|binary}.c,
> > > like indexes.
> > > How do other think?
> >
> > Not sure about this, it seems others have put a lot of effort into
> > splitting TO and From.
> > Also like to hear from others.
> >
> > >
> > > 07. fmt_to_name()
> > >
> > > I'm not sure the function is really needed. Can we follow like get_foreign_data_wrapper_oid()
> > > and remove the funciton?
> >
> > I have referenced some code from greenplum, will remove this.
> >
> > >
> > > 08. GetCopyRoutineByName()
> > >
> > > Should we use syscache for searching a catalog?
> > >
> > > 09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()
> > >
> > > Comments still refer CopyHandlerOps, whereas it was renamed.
> > >
> > > 10. copy.h
> > >
> > > Per foreign.h and fdwapi.h, should we add a new header file and move some APIs?
> > >
> > > 11. copy.h
> > >
> > > ```
> > > -/* These are private in commands/copy[from|to].c */
> > > -typedef struct CopyFromStateData *CopyFromState;
> > > -typedef struct CopyToStateData *CopyToState;
> > > ```
> > >
> > > Are above changes really needed?
> > >
> > > 12. CopyFormatOptions
> > >
> > > Can we remove `bool binary` in future?
> > >
> > > 13. external functions
> > >
> > > ```
> > > +extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
> > > +extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot *slot);
> > > +extern void CopyToFormatTextEnd(CopyToState cstate);
> > > +extern void CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc);
> > > +extern bool CopyFromFormatTextNext(CopyFromState cstate, ExprContext *econtext,
> > > +
> > > Datum *values, bool *nulls);
> > > +extern void CopyFromFormatTextErrorCallback(CopyFromState cstate);
> > > +
> > > +extern void CopyToFormatBinaryStart(CopyToState cstate, TupleDesc tupDesc);
> > > +extern void CopyToFormatBinaryOneRow(CopyToState cstate, TupleTableSlot *slot);
> > > +extern void CopyToFormatBinaryEnd(CopyToState cstate);
> > > +extern void CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc);
> > > +extern bool CopyFromFormatBinaryNext(CopyFromState cstate,
> > > ExprContext *econtext,
> > > +
> > > Datum *values, bool *nulls);
> > > +extern void CopyFromFormatBinaryErrorCallback(CopyFromState cstate);
> > > ```
> > >
> > > FYI - If you add files for {text|csv|binary}, these declarations can be removed.
> > >
> > > Best Regards,
> > > Hayato Kuroda
> > > FUJITSU LIMITED
> > >
> >
> > Thanks for all the valuable suggestions.
> >
> > --
> > Regards
> > Junwang Zhao
> >
> >

--
Regards
Junwang Zhao

On Mon, Dec 11, 2023 at 7:19 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Dec 11, 2023 at 10:57:15AM +0900, Masahiko Sawada wrote:
> > IIUC we cannot create two same name functions with the same arguments
> > but a different return value type in the first place. It seems to me
> > to be an overkill to change such a design.
>
> Agreed to not touch the logictics of LookupFuncName() for the sake of
> this thread. I have not checked the SQL specification, but I recall
> that there are a few assumptions from the spec embedded in the lookup
> logic particularly when it comes to specify a procedure name without
> arguments.
>
> > Another idea is to encapsulate copy_to/from_handler by a super class
> > like copy_handler. The handler function is called with an argument,
> > say copyto, and returns copy_handler encapsulating either
> > copy_to/from_handler depending on the argument.
>
> Yep, that's possible as well and can work as a cross-check between the
> argument and the NodeTag assigned to the handler structure returned by
> the function.
>
> At the end, the final result of the patch should IMO include:
> - Documentation about how one can register a custom copy_handler.
> - Something in src/test/modules/, minimalistic still useful that can
> be used as a template when one wants to implement their own handler.
> The documentation should mention about this module.
> - No need for SQL functions for all the in-core handlers: let's just
> return pointers to them based on the options given.

Agreed.

> It would be probably cleaner to split the patch so as the code is
> refactored and evaluated with the in-core handlers first, and then
> extended with the pluggable facilities and the function lookups.

Agreed.

I've sketched the above idea including a test module in
src/test/module/test_copy_format, based on v2 patch. It's not splitted
and is dirty so just for discussion.

Regards,

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

On Mon, Dec 11, 2023 at 10:32 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Dec 11, 2023 at 7:19 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Mon, Dec 11, 2023 at 10:57:15AM +0900, Masahiko Sawada wrote:
> > > IIUC we cannot create two same name functions with the same arguments
> > > but a different return value type in the first place. It seems to me
> > > to be an overkill to change such a design.
> >
> > Agreed to not touch the logictics of LookupFuncName() for the sake of
> > this thread. I have not checked the SQL specification, but I recall
> > that there are a few assumptions from the spec embedded in the lookup
> > logic particularly when it comes to specify a procedure name without
> > arguments.
> >
> > > Another idea is to encapsulate copy_to/from_handler by a super class
> > > like copy_handler. The handler function is called with an argument,
> > > say copyto, and returns copy_handler encapsulating either
> > > copy_to/from_handler depending on the argument.
> >
> > Yep, that's possible as well and can work as a cross-check between the
> > argument and the NodeTag assigned to the handler structure returned by
> > the function.
> >
> > At the end, the final result of the patch should IMO include:
> > - Documentation about how one can register a custom copy_handler.
> > - Something in src/test/modules/, minimalistic still useful that can
> > be used as a template when one wants to implement their own handler.
> > The documentation should mention about this module.
> > - No need for SQL functions for all the in-core handlers: let's just
> > return pointers to them based on the options given.
>
> Agreed.
>
> > It would be probably cleaner to split the patch so as the code is
> > refactored and evaluated with the in-core handlers first, and then
> > extended with the pluggable facilities and the function lookups.
>
> Agreed.
>
> I've sketched the above idea including a test module in
> src/test/module/test_copy_format, based on v2 patch. It's not splitted
> and is dirty so just for discussion.
>
The test_copy_format extension doesn't use the fields of CopyToState and
CopyFromState in this patch, I think we should move CopyFromStateData
and CopyToStateData to commands/copy.h, what do you think?

The framework in the patch LGTM.

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

--
Regards
Junwang Zhao

Dear Sutou-san, Junwang,

Sorry for the delay reply.

>
> Can we discuss how to proceed this improvement?
>
> There are 2 approaches for it:
>
> 1. Do the followings concurrently:
> a. Implementing small changes that got a consensus and
> merge them step-by-step
> (e.g. We got a consensus that we need to extract the
> current format related routines.)
> b. Discuss design
>
> (v1-v3 patches use this approach.)
>
> 2. Implement one (large) complete patch set with design
> discussion and merge it
>
> (v4- patches use this approach.)
>
> Which approach is preferred? (Or should we choose another
> approach?)
>
> I thought that 1. is preferred because it will reduce review
> cost. So I chose 1.

I'm ok to use approach 1, but could you please divide a large patch? E.g.,

0001. defines an infrastructure for copy-API
0002. adjusts current codes to use APIs
0003. adds a test module in src/test/modules or contrib.
...

This approach helps reviewers to see patches deeper. Separated patches can be
combined when they are close to committable.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Dec 12, 2023 at 11:09 AM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> On Mon, Dec 11, 2023 at 10:32 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Dec 11, 2023 at 7:19 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > >
> > > On Mon, Dec 11, 2023 at 10:57:15AM +0900, Masahiko Sawada wrote:
> > > > IIUC we cannot create two same name functions with the same arguments
> > > > but a different return value type in the first place. It seems to me
> > > > to be an overkill to change such a design.
> > >
> > > Agreed to not touch the logictics of LookupFuncName() for the sake of
> > > this thread. I have not checked the SQL specification, but I recall
> > > that there are a few assumptions from the spec embedded in the lookup
> > > logic particularly when it comes to specify a procedure name without
> > > arguments.
> > >
> > > > Another idea is to encapsulate copy_to/from_handler by a super class
> > > > like copy_handler. The handler function is called with an argument,
> > > > say copyto, and returns copy_handler encapsulating either
> > > > copy_to/from_handler depending on the argument.
> > >
> > > Yep, that's possible as well and can work as a cross-check between the
> > > argument and the NodeTag assigned to the handler structure returned by
> > > the function.
> > >
> > > At the end, the final result of the patch should IMO include:
> > > - Documentation about how one can register a custom copy_handler.
> > > - Something in src/test/modules/, minimalistic still useful that can
> > > be used as a template when one wants to implement their own handler.
> > > The documentation should mention about this module.
> > > - No need for SQL functions for all the in-core handlers: let's just
> > > return pointers to them based on the options given.
> >
> > Agreed.
> >
> > > It would be probably cleaner to split the patch so as the code is
> > > refactored and evaluated with the in-core handlers first, and then
> > > extended with the pluggable facilities and the function lookups.
> >
> > Agreed.
> >
> > I've sketched the above idea including a test module in
> > src/test/module/test_copy_format, based on v2 patch. It's not splitted
> > and is dirty so just for discussion.
> >
> The test_copy_format extension doesn't use the fields of CopyToState and
> CopyFromState in this patch, I think we should move CopyFromStateData
> and CopyToStateData to commands/copy.h, what do you think?

Yes, I basically agree with that, where we move CopyFromStateData to
might depend on how we define COPY FROM APIs though. I think we can
move CopyToStateData to copy.h at least.

Regards,

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

Hi,

In <CAD21AoCvjGserrtEU=UcA3Mfyfe6ftf9OXPHv9fiJ9DmXMJ2nQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Dec 2023 10:57:15 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> IIUC we cannot create two same name functions with the same arguments
> but a different return value type in the first place. It seems to me
> to be an overkill to change such a design.

Oh, sorry. I didn't notice it.

> Another idea is to encapsulate copy_to/from_handler by a super class
> like copy_handler. The handler function is called with an argument,
> say copyto, and returns copy_handler encapsulating either
> copy_to/from_handler depending on the argument.

It's for using "${copy_format_name}" such as "json" and
"parquet" as a function name, right? If we use the
"${copy_format_name}" approach, we can't use function names
that are already used by tablesample method handler such as
"system" and "bernoulli" for COPY FORMAT name. Because both
of tablesample method handler function and COPY FORMAT
handler function use "(internal)" as arguments.

I think that tablesample method names and COPY FORMAT names
will not be conflicted but the limitation (using the same
namespace for tablesample method and COPY FORMAT) is
unnecessary limitation.

How about using prefix ("copy_to_${copy_format_name}" or
something) or suffix ("${copy_format_name}_copy_to" or
something) for function names? For example,
"copy_to_json"/"copy_from_json" for "json" COPY FORMAT.

("copy_${copy_format_name}" that returns copy_handler
encapsulating either copy_to/from_handler depending on the
argument may be an option.)

Thanks,
--
kou

On Thu, Dec 14, 2023 at 6:44 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoCvjGserrtEU=UcA3Mfyfe6ftf9OXPHv9fiJ9DmXMJ2nQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Dec 2023 10:57:15 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > IIUC we cannot create two same name functions with the same arguments
> > but a different return value type in the first place. It seems to me
> > to be an overkill to change such a design.
>
> Oh, sorry. I didn't notice it.
>
> > Another idea is to encapsulate copy_to/from_handler by a super class
> > like copy_handler. The handler function is called with an argument,
> > say copyto, and returns copy_handler encapsulating either
> > copy_to/from_handler depending on the argument.
>
> It's for using "${copy_format_name}" such as "json" and
> "parquet" as a function name, right?

Right.

> If we use the
> "${copy_format_name}" approach, we can't use function names
> that are already used by tablesample method handler such as
> "system" and "bernoulli" for COPY FORMAT name. Because both
> of tablesample method handler function and COPY FORMAT
> handler function use "(internal)" as arguments.
>
> I think that tablesample method names and COPY FORMAT names
> will not be conflicted but the limitation (using the same
> namespace for tablesample method and COPY FORMAT) is
> unnecessary limitation.

Presumably, such function name collisions are not limited to
tablesample and copy, but apply to all functions that have an
"internal" argument. To avoid collisions, extensions can be created in
a different schema than public. And note that built-in format copy
handler doesn't need to declare its handler function.

>
> How about using prefix ("copy_to_${copy_format_name}" or
> something) or suffix ("${copy_format_name}_copy_to" or
> something) for function names? For example,
> "copy_to_json"/"copy_from_json" for "json" COPY FORMAT.
>
> ("copy_${copy_format_name}" that returns copy_handler
> encapsulating either copy_to/from_handler depending on the
> argument may be an option.)

While there is a way to avoid collision as I mentioned above, I can
see the point that we might want to avoid using a generic function
name such as "arrow" and "parquet" as custom copy handler functions.
Adding a prefix or suffix would be one option but to give extensions
more flexibility, another option would be to support format = 'custom'
and add the "handler" option to specify a copy handler function name
to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
HANDLER = 'arrow_copy_handler').

Regards,

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

Hi,

In <CAD21AoCZv3cVU+NxR2s9J_dWvjrS350GFFr2vMgCH8wWxQ5hTQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 15 Dec 2023 05:19:43 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> To avoid collisions, extensions can be created in a
> different schema than public.

Thanks. I didn't notice it.

> And note that built-in format copy handler doesn't need to
> declare its handler function.

Right. I know it.

> Adding a prefix or suffix would be one option but to give extensions
> more flexibility, another option would be to support format = 'custom'
> and add the "handler" option to specify a copy handler function name
> to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
> HANDLER = 'arrow_copy_handler').

Interesting. If we use this option, users can choose an COPY
FORMAT implementation they like from multiple
implementations. For example, a developer may implement a
COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
related API and another developer may implement a handler
with simdjson[1] which is a fast JSON parser. Users can
choose whichever they like.

But specifying HANDLER = '...' explicitly is a bit
inconvenient. Because only one handler will be installed in
most use cases. In the case, users don't need to choose one
handler.

If we choose this option, it may be better that we also
provide a mechanism that can work without HANDLER. Searching
a function by name like tablesample method does is an option.

[1]: https://github.com/simdjson/simdjson

Thanks,
--
kou

Hi,

In
<OS3PR01MB9882F023300EDC5AFD8A8339F58EA(at)OS3PR01MB9882(dot)jpnprd01(dot)prod(dot)outlook(dot)com>
"RE: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 12 Dec 2023 02:31:53 +0000,
"Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:

>> Can we discuss how to proceed this improvement?
>>
>> There are 2 approaches for it:
>>
>> 1. Do the followings concurrently:
>> a. Implementing small changes that got a consensus and
>> merge them step-by-step
>> (e.g. We got a consensus that we need to extract the
>> current format related routines.)
>> b. Discuss design
>>
>> (v1-v3 patches use this approach.)
>>
>> 2. Implement one (large) complete patch set with design
>> discussion and merge it
>>
>> (v4- patches use this approach.)
>>
>> Which approach is preferred? (Or should we choose another
>> approach?)
>>
>> I thought that 1. is preferred because it will reduce review
>> cost. So I chose 1.
>
> I'm ok to use approach 1, but could you please divide a large patch? E.g.,
>
> 0001. defines an infrastructure for copy-API
> 0002. adjusts current codes to use APIs
> 0003. adds a test module in src/test/modules or contrib.
> ...
>
> This approach helps reviewers to see patches deeper. Separated patches can be
> combined when they are close to committable.

It seems that I should have chosen another approach based on
comments so far:

3. Do the followings in order:
a. Implement a workable (but maybe dirty and/or incomplete)
implementation to discuss design like [1], discuss
design with it and get a consensus on design
b. Implement small patches based on the design

[1]: https://www.postgresql.org/message-id/CAD21AoCunywHird3GaPzWe6s9JG1wzxj3Cr6vGN36DDheGjOjA%40mail.gmail.com

I'll implement a custom COPY FORMAT handler with [1] and
provide a feedback with the experience. (It's for a.)

Thanks,
--
kou

On Fri, Dec 15, 2023 at 8:53 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoCZv3cVU+NxR2s9J_dWvjrS350GFFr2vMgCH8wWxQ5hTQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 15 Dec 2023 05:19:43 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > To avoid collisions, extensions can be created in a
> > different schema than public.
>
> Thanks. I didn't notice it.
>
> > And note that built-in format copy handler doesn't need to
> > declare its handler function.
>
> Right. I know it.
>
> > Adding a prefix or suffix would be one option but to give extensions
> > more flexibility, another option would be to support format = 'custom'
> > and add the "handler" option to specify a copy handler function name
> > to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
> > HANDLER = 'arrow_copy_handler').
>
I like the prefix/suffix idea, easy to implement. *custom* is not a FORMAT,
and user has to know the name of the specific handler names, not
intuitive.

> Interesting. If we use this option, users can choose an COPY
> FORMAT implementation they like from multiple
> implementations. For example, a developer may implement a
> COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
> related API and another developer may implement a handler
> with simdjson[1] which is a fast JSON parser. Users can
> choose whichever they like.
Not sure about this, why not move Json copy handler to contrib
as an example for others, any extensions share the same format
function name and just install one? No bound would implement
another CSV or TEXT copy handler IMHO.
>
> But specifying HANDLER = '...' explicitly is a bit
> inconvenient. Because only one handler will be installed in
> most use cases. In the case, users don't need to choose one
> handler.
>
> If we choose this option, it may be better that we also
> provide a mechanism that can work without HANDLER. Searching
> a function by name like tablesample method does is an option.
>
>
> [1]: https://github.com/simdjson/simdjson
>
>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

On Fri, Dec 15, 2023 at 9:53 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoCZv3cVU+NxR2s9J_dWvjrS350GFFr2vMgCH8wWxQ5hTQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 15 Dec 2023 05:19:43 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > To avoid collisions, extensions can be created in a
> > different schema than public.
>
> Thanks. I didn't notice it.
>
> > And note that built-in format copy handler doesn't need to
> > declare its handler function.
>
> Right. I know it.
>
> > Adding a prefix or suffix would be one option but to give extensions
> > more flexibility, another option would be to support format = 'custom'
> > and add the "handler" option to specify a copy handler function name
> > to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
> > HANDLER = 'arrow_copy_handler').
>
> Interesting. If we use this option, users can choose an COPY
> FORMAT implementation they like from multiple
> implementations. For example, a developer may implement a
> COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
> related API and another developer may implement a handler
> with simdjson[1] which is a fast JSON parser. Users can
> choose whichever they like.
>
> But specifying HANDLER = '...' explicitly is a bit
> inconvenient. Because only one handler will be installed in
> most use cases. In the case, users don't need to choose one
> handler.
>
> If we choose this option, it may be better that we also
> provide a mechanism that can work without HANDLER. Searching
> a function by name like tablesample method does is an option.

Agreed. We can search the function by format name by default and the
user can optionally specify the handler function name in case where
the names of the installed custom copy handler collide. Probably the
handler option stuff could be a follow-up patch.

Regards,

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

Hi,

In <CAEG8a3JuShA6g19Nt_Ejk15BrNA6PmeCbK7p81izZi71muGq3g(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 15 Dec 2023 11:27:30 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

>> > Adding a prefix or suffix would be one option but to give extensions
>> > more flexibility, another option would be to support format = 'custom'
>> > and add the "handler" option to specify a copy handler function name
>> > to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
>> > HANDLER = 'arrow_copy_handler').
>>
> I like the prefix/suffix idea, easy to implement. *custom* is not a FORMAT,
> and user has to know the name of the specific handler names, not
> intuitive.

Ah! I misunderstood this idea. "custom" is the special
format to use "HANDLER". I thought that we can use it like

(FORMAT = 'arrow', HANDLER = 'arrow_copy_handler_impl1')

and

(FORMAT = 'arrow', HANDLER = 'arrow_copy_handler_impl2')

.

>> Interesting. If we use this option, users can choose an COPY
>> FORMAT implementation they like from multiple
>> implementations. For example, a developer may implement a
>> COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
>> related API and another developer may implement a handler
>> with simdjson[1] which is a fast JSON parser. Users can
>> choose whichever they like.
> Not sure about this, why not move Json copy handler to contrib
> as an example for others, any extensions share the same format
> function name and just install one? No bound would implement
> another CSV or TEXT copy handler IMHO.

I should have used a different format not JSON as an example
for easy to understand. I just wanted to say that extension
developers can implement another implementation without
conflicting another implementation.

Thanks,
--
kou

On Fri, Dec 15, 2023 at 12:45 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3JuShA6g19Nt_Ejk15BrNA6PmeCbK7p81izZi71muGq3g(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 15 Dec 2023 11:27:30 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> >> > Adding a prefix or suffix would be one option but to give extensions
> >> > more flexibility, another option would be to support format = 'custom'
> >> > and add the "handler" option to specify a copy handler function name
> >> > to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
> >> > HANDLER = 'arrow_copy_handler').
> >>
> > I like the prefix/suffix idea, easy to implement. *custom* is not a FORMAT,
> > and user has to know the name of the specific handler names, not
> > intuitive.
>
> Ah! I misunderstood this idea. "custom" is the special
> format to use "HANDLER". I thought that we can use it like
>
> (FORMAT = 'arrow', HANDLER = 'arrow_copy_handler_impl1')
>
> and
>
> (FORMAT = 'arrow', HANDLER = 'arrow_copy_handler_impl2')
>
> .
>
> >> Interesting. If we use this option, users can choose an COPY
> >> FORMAT implementation they like from multiple
> >> implementations. For example, a developer may implement a
> >> COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
> >> related API and another developer may implement a handler
> >> with simdjson[1] which is a fast JSON parser. Users can
> >> choose whichever they like.
> > Not sure about this, why not move Json copy handler to contrib
> > as an example for others, any extensions share the same format
> > function name and just install one? No bound would implement
> > another CSV or TEXT copy handler IMHO.
>
> I should have used a different format not JSON as an example
> for easy to understand. I just wanted to say that extension
> developers can implement another implementation without
> conflicting another implementation.

Yeah, I can see the value of the HANDLER option now. The possibility
of two extensions for the same format using same hanlder name should
be rare I guess ;)
>
>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

Hi,

In <CAD21AoCunywHird3GaPzWe6s9JG1wzxj3Cr6vGN36DDheGjOjA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Dec 2023 23:31:29 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> I've sketched the above idea including a test module in
> src/test/module/test_copy_format, based on v2 patch. It's not splitted
> and is dirty so just for discussion.

I implemented a sample COPY TO handler for Apache Arrow that
supports only integer and text.

I needed to extend the patch:

1. Add an opaque space for custom COPY TO handler
* Add CopyToState{Get,Set}Opaque()
https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944

2. Export CopyToState::attnumlist
* Add CopyToStateGetAttNumList()
https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688

3. Export CopySend*()
* Rename CopySend*() to CopyToStateSend*() and export them
* Exception: CopySendEndOfRow() to CopyToStateFlush() because
it just flushes the internal buffer now.
https://github.com/kou/postgres/commit/289a5640135bde6733a1b8e2c412221ad522901e

The attached patch is based on the Sawada-san's patch and
includes the above changes. Note that this patch is also
dirty so just for discussion.

My suggestions from this experience:

1. Split COPY handler to COPY TO handler and COPY FROM handler

* CopyFormatRoutine is a bit tricky. An extension needs
to create a CopyFormatRoutine node and
a CopyToFormatRoutine node.

* If we just require "copy_to_${FORMAT}(internal)"
function and "copy_from_${FORMAT}(internal)" function,
we can remove the tricky approach. And it also avoid
name collisions with other handler such as tablesample
handler.
See also:
https://www.postgresql.org/message-id/flat/20231214.184414.2179134502876898942.kou%40clear-code.com#af71f364d0a9f5c144e45b447e5c16c9

2. Need an opaque space like IndexScanDesc::opaque does

* A custom COPY TO handler needs to keep its data

3. Export CopySend*()

* If we like minimum API, we just need to export
CopySendData() and CopySendEndOfRow(). But
CopySend{String,Char,Int32,Int16}() will be convenient
custom COPY TO handlers. (A custom COPY TO handler for
Apache Arrow doesn't need them.)

Questions:

1. What value should be used for "format" in
PgMsg_CopyOutResponse message?

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/copyto.c;h=c66a047c4a79cc614784610f385f1cd0935350f3;hb=9ca6e7b9411e36488ef539a2c1f6846ac92a7072#l144

It's 1 for binary format and 0 for text/csv format.

Should we make it customizable by custom COPY TO handler?
If so, what value should be used for this?

2. Do we need more tries for design discussion for the first
implementation? If we need, what should we try?

Thanks,
--
kou

On Thu, Dec 21, 2023 at 06:35:04PM +0900, Sutou Kouhei wrote:
> * If we just require "copy_to_${FORMAT}(internal)"
> function and "copy_from_${FORMAT}(internal)" function,
> we can remove the tricky approach. And it also avoid
> name collisions with other handler such as tablesample
> handler.
> See also:
> https://www.postgresql.org/message-id/flat/20231214.184414.2179134502876898942.kou%40clear-code.com#af71f364d0a9f5c144e45b447e5c16c9

Hmm. I prefer the unique name approach for the COPY portions without
enforcing any naming policy on the function names returning the
handlers, actually, though I can see your point.

> 2. Need an opaque space like IndexScanDesc::opaque does
>
> * A custom COPY TO handler needs to keep its data

Sounds useful to me to have a private area passed down to the
callbacks.

> 3. Export CopySend*()
>
> * If we like minimum API, we just need to export
> CopySendData() and CopySendEndOfRow(). But
> CopySend{String,Char,Int32,Int16}() will be convenient
> custom COPY TO handlers. (A custom COPY TO handler for
> Apache Arrow doesn't need them.)

Hmm. Not sure on this one. This may come down to externalize the
manipulation of fe_msgbuf. Particularly, could it be possible that
some custom formats don't care at all about the network order?

> Questions:
>
> 1. What value should be used for "format" in
> PgMsg_CopyOutResponse message?
>
> It's 1 for binary format and 0 for text/csv format.
>
> Should we make it customizable by custom COPY TO handler?
> If so, what value should be used for this?

Interesting point. It looks very tempting to give more flexibility to
people who'd like to use their own code as we have one byte in the
protocol but just use 0/1. Hence it feels natural to have a callback
for that.

It also means that we may want to think harder about copy_is_binary in
libpq in the future step. Now, having a backend implementation does
not need any libpq bits, either, because a client stack may just want
to speak the Postgres protocol directly. Perhaps a custom COPY
implementation would be OK with how things are in libpq, as well,
tweaking its way through with just text or binary.

> 2. Do we need more tries for design discussion for the first
> implementation? If we need, what should we try?

A makeNode() is used with an allocation in the current memory context
in the function returning the handler. I would have assume that this
stuff returns a handler as a const struct like table AMs.
--
Michael

On Fri, Dec 22, 2023 at 10:00 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Dec 21, 2023 at 06:35:04PM +0900, Sutou Kouhei wrote:
> > * If we just require "copy_to_${FORMAT}(internal)"
> > function and "copy_from_${FORMAT}(internal)" function,
> > we can remove the tricky approach. And it also avoid
> > name collisions with other handler such as tablesample
> > handler.
> > See also:
> > https://www.postgresql.org/message-id/flat/20231214.184414.2179134502876898942.kou%40clear-code.com#af71f364d0a9f5c144e45b447e5c16c9
>
> Hmm. I prefer the unique name approach for the COPY portions without
> enforcing any naming policy on the function names returning the
> handlers, actually, though I can see your point.

Yeah, another idea is to provide support functions to return a
CopyFormatRoutine wrapping either CopyToFormatRoutine or
CopyFromFormatRoutine. For example:

extern CopyFormatRoutine *MakeCopyToFormatRoutine(const
CopyToFormatRoutine *routine);

extensions can do like:

static const CopyToFormatRoutine testfmt_handler = {
.type = T_CopyToFormatRoutine,
.start_fn = testfmt_copyto_start,
.onerow_fn = testfmt_copyto_onerow,
.end_fn = testfmt_copyto_end
};

Datum
copy_testfmt_handler(PG_FUNCTION_ARGS)
{
CopyFormatRoutine *routine = MakeCopyToFormatRoutine(&testfmt_handler);
:

>
> > 2. Need an opaque space like IndexScanDesc::opaque does
> >
> > * A custom COPY TO handler needs to keep its data
>
> Sounds useful to me to have a private area passed down to the
> callbacks.
>

+1

>
> > Questions:
> >
> > 1. What value should be used for "format" in
> > PgMsg_CopyOutResponse message?
> >
> > It's 1 for binary format and 0 for text/csv format.
> >
> > Should we make it customizable by custom COPY TO handler?
> > If so, what value should be used for this?
>
> Interesting point. It looks very tempting to give more flexibility to
> people who'd like to use their own code as we have one byte in the
> protocol but just use 0/1. Hence it feels natural to have a callback
> for that.

+1

>
> It also means that we may want to think harder about copy_is_binary in
> libpq in the future step. Now, having a backend implementation does
> not need any libpq bits, either, because a client stack may just want
> to speak the Postgres protocol directly. Perhaps a custom COPY
> implementation would be OK with how things are in libpq, as well,
> tweaking its way through with just text or binary.
>
> > 2. Do we need more tries for design discussion for the first
> > implementation? If we need, what should we try?
>
> A makeNode() is used with an allocation in the current memory context
> in the function returning the handler. I would have assume that this
> stuff returns a handler as a const struct like table AMs.

+1

The example I mentioned above does that.

Regards,

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

On Thu, Dec 21, 2023 at 6:35 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoCunywHird3GaPzWe6s9JG1wzxj3Cr6vGN36DDheGjOjA(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Dec 2023 23:31:29 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > I've sketched the above idea including a test module in
> > src/test/module/test_copy_format, based on v2 patch. It's not splitted
> > and is dirty so just for discussion.
>
> I implemented a sample COPY TO handler for Apache Arrow that
> supports only integer and text.
>
> I needed to extend the patch:
>
> 1. Add an opaque space for custom COPY TO handler
> * Add CopyToState{Get,Set}Opaque()
> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
>
> 2. Export CopyToState::attnumlist
> * Add CopyToStateGetAttNumList()
> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688

I think we can move CopyToState to copy.h and we don't need to have
set/get functions for its fields.

Regards,

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

On Thu, Dec 21, 2023 at 5:35 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoCunywHird3GaPzWe6s9JG1wzxj3Cr6vGN36DDheGjOjA(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Dec 2023 23:31:29 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > I've sketched the above idea including a test module in
> > src/test/module/test_copy_format, based on v2 patch. It's not splitted
> > and is dirty so just for discussion.
>
> I implemented a sample COPY TO handler for Apache Arrow that
> supports only integer and text.
>
> I needed to extend the patch:
>
> 1. Add an opaque space for custom COPY TO handler
> * Add CopyToState{Get,Set}Opaque()
> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
>
> 2. Export CopyToState::attnumlist
> * Add CopyToStateGetAttNumList()
> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688
>
> 3. Export CopySend*()
> * Rename CopySend*() to CopyToStateSend*() and export them
> * Exception: CopySendEndOfRow() to CopyToStateFlush() because
> it just flushes the internal buffer now.
> https://github.com/kou/postgres/commit/289a5640135bde6733a1b8e2c412221ad522901e
>
I guess the purpose of these helpers is to avoid expose CopyToState to
copy.h, but I
think expose CopyToState to user might make life easier, users might want to use
the memory contexts of the structure (though I agree not all the
fields are necessary
for extension handers).

> The attached patch is based on the Sawada-san's patch and
> includes the above changes. Note that this patch is also
> dirty so just for discussion.
>
> My suggestions from this experience:
>
> 1. Split COPY handler to COPY TO handler and COPY FROM handler
>
> * CopyFormatRoutine is a bit tricky. An extension needs
> to create a CopyFormatRoutine node and
> a CopyToFormatRoutine node.
>
> * If we just require "copy_to_${FORMAT}(internal)"
> function and "copy_from_${FORMAT}(internal)" function,
> we can remove the tricky approach. And it also avoid
> name collisions with other handler such as tablesample
> handler.
> See also:
> https://www.postgresql.org/message-id/flat/20231214.184414.2179134502876898942.kou%40clear-code.com#af71f364d0a9f5c144e45b447e5c16c9
>
> 2. Need an opaque space like IndexScanDesc::opaque does
>
> * A custom COPY TO handler needs to keep its data

I once thought users might want to parse their own options, maybe this
is a use case for this opaque space.

For the name, I thought private_data might be a better candidate than
opaque, but I do not insist.
>
> 3. Export CopySend*()
>
> * If we like minimum API, we just need to export
> CopySendData() and CopySendEndOfRow(). But
> CopySend{String,Char,Int32,Int16}() will be convenient
> custom COPY TO handlers. (A custom COPY TO handler for
> Apache Arrow doesn't need them.)

Do you use the arrow library to control the memory? Is there a way that
we can let the arrow use postgres' memory context? I'm not sure this
is necessary, just raise the question for discussion.
>
> Questions:
>
> 1. What value should be used for "format" in
> PgMsg_CopyOutResponse message?
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/copyto.c;h=c66a047c4a79cc614784610f385f1cd0935350f3;hb=9ca6e7b9411e36488ef539a2c1f6846ac92a7072#l144
>
> It's 1 for binary format and 0 for text/csv format.
>
> Should we make it customizable by custom COPY TO handler?
> If so, what value should be used for this?
>
> 2. Do we need more tries for design discussion for the first
> implementation? If we need, what should we try?
>
>
> Thanks,
> --
> kou

+PG_FUNCTION_INFO_V1(copy_testfmt_handler);
+Datum
+copy_testfmt_handler(PG_FUNCTION_ARGS)
+{
+ bool is_from = PG_GETARG_BOOL(0);
+ CopyFormatRoutine *cp = makeNode(CopyFormatRoutine);;
+

extra semicolon.

--
Regards
Junwang Zhao

Hi,

In <ZYTfqGppMc9e_w2k(at)paquier(dot)xyz>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Dec 2023 10:00:24 +0900,
Michael Paquier <michael(at)paquier(dot)xyz> wrote:

>> 3. Export CopySend*()
>>
>> * If we like minimum API, we just need to export
>> CopySendData() and CopySendEndOfRow(). But
>> CopySend{String,Char,Int32,Int16}() will be convenient
>> custom COPY TO handlers. (A custom COPY TO handler for
>> Apache Arrow doesn't need them.)
>
> Hmm. Not sure on this one. This may come down to externalize the
> manipulation of fe_msgbuf. Particularly, could it be possible that
> some custom formats don't care at all about the network order?

It means that all custom formats should control byte order
by themselves instead of using CopySendInt*() that always
use network byte order, right? It makes sense. Let's export
only CopySendData() and CopySendEndOfRow().

>> 1. What value should be used for "format" in
>> PgMsg_CopyOutResponse message?
>>
>> It's 1 for binary format and 0 for text/csv format.
>>
>> Should we make it customizable by custom COPY TO handler?
>> If so, what value should be used for this?
>
> Interesting point. It looks very tempting to give more flexibility to
> people who'd like to use their own code as we have one byte in the
> protocol but just use 0/1. Hence it feels natural to have a callback
> for that.

OK. Let's add a callback something like:

typedef int16 (*CopyToGetFormat_function) (CopyToState cstate);

> It also means that we may want to think harder about copy_is_binary in
> libpq in the future step. Now, having a backend implementation does
> not need any libpq bits, either, because a client stack may just want
> to speak the Postgres protocol directly. Perhaps a custom COPY
> implementation would be OK with how things are in libpq, as well,
> tweaking its way through with just text or binary.

Can we defer this discussion after we commit a basic custom
COPY format handler mechanism?

>> 2. Do we need more tries for design discussion for the first
>> implementation? If we need, what should we try?
>
> A makeNode() is used with an allocation in the current memory context
> in the function returning the handler. I would have assume that this
> stuff returns a handler as a const struct like table AMs.

If we use this approach, we can't use the Sawada-san's
idea[1] that provides a convenient API to hide
CopyFormatRoutine internal. The idea provides
MakeCopy{To,From}FormatRoutine(). They return a new
CopyFormatRoutine* with suitable is_from member. They can't
use static const CopyFormatRoutine because they may be called
multiple times in the same process.

We can use the satic const struct approach by choosing one
of the followings:

1. Use separated function for COPY {TO,FROM} format handlers
as I suggested.

2. Don't provide convenient API. Developers construct
CopyFormatRoutine by themselves. But it may be a bit
tricky.

3. Similar to 2. but don't use a bit tricky approach (don't
embed Copy{To,From}FormatRoutine nodes into
CopyFormatRoutine).

Use unified function for COPY {TO,FROM} format handlers
but CopyFormatRoutine always have both of COPY {TO,FROM}
format routines and these routines aren't nodes:

typedef struct CopyToFormatRoutine
{
CopyToStart_function start_fn;
CopyToOneRow_function onerow_fn;
CopyToEnd_function end_fn;
} CopyToFormatRoutine;

/* XXX: just copied from COPY TO routines */
typedef struct CopyFromFormatRoutine
{
CopyFromStart_function start_fn;
CopyFromOneRow_function onerow_fn;
CopyFromEnd_function end_fn;
} CopyFromFormatRoutine;

typedef struct CopyFormatRoutine
{
NodeTag type;

CopyToFormatRoutine to_routine;
CopyFromFormatRoutine from_routine;
} CopyFormatRoutine;

----

static const CopyFormatRoutine testfmt_handler = {
.type = T_CopyFormatRoutine,
.to_routine = {
.start_fn = testfmt_copyto_start,
.onerow_fn = testfmt_copyto_onerow,
.end_fn = testfmt_copyto_end,
},
.from_routine = {
.start_fn = testfmt_copyfrom_start,
.onerow_fn = testfmt_copyfrom_onerow,
.end_fn = testfmt_copyfrom_end,
},
};

PG_FUNCTION_INFO_V1(copy_testfmt_handler);
Datum
copy_testfmt_handler(PG_FUNCTION_ARGS)
{
PG_RETURN_POINTER(&testfmt_handler);
}

4. ... other idea?

[1] https://www.postgresql.org/message-id/flat/CAD21AoDs9cOjuVbA_krGizAdc50KE%2BFjAuEXWF0NZwbMnc7F3Q%40mail.gmail.com#71bb03d9237252382b245dd33e705a3a

Thanks,
--
kou

Hi,

In <CAD21AoD=UapH4Wh06G6H5XAzPJ0iJg9YcW8r7E2UEJkZ8QsosA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Dec 2023 10:48:18 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

>> I needed to extend the patch:
>>
>> 1. Add an opaque space for custom COPY TO handler
>> * Add CopyToState{Get,Set}Opaque()
>> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
>>
>> 2. Export CopyToState::attnumlist
>> * Add CopyToStateGetAttNumList()
>> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688
>
> I think we can move CopyToState to copy.h and we don't need to have
> set/get functions for its fields.

I don't object the idea if other PostgreSQL developers
prefer the approach. Is there any PostgreSQL developer who
objects that we export Copy{To,From}StateData as public API?

Thanks,
--
kou

Hi,

In <CAEG8a3+jG_NKOUmcxDyEX2xSggBXReZ4H=e3RFsUtedY88A03w(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Dec 2023 10:58:05 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

>> 1. Add an opaque space for custom COPY TO handler
>> * Add CopyToState{Get,Set}Opaque()
>> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
>>
>> 2. Export CopyToState::attnumlist
>> * Add CopyToStateGetAttNumList()
>> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688
>>
>> 3. Export CopySend*()
>> * Rename CopySend*() to CopyToStateSend*() and export them
>> * Exception: CopySendEndOfRow() to CopyToStateFlush() because
>> it just flushes the internal buffer now.
>> https://github.com/kou/postgres/commit/289a5640135bde6733a1b8e2c412221ad522901e
>>
> I guess the purpose of these helpers is to avoid expose CopyToState to
> copy.h,

Yes.

> but I
> think expose CopyToState to user might make life easier, users might want to use
> the memory contexts of the structure (though I agree not all the
> fields are necessary
> for extension handers).

OK. I don't object it as I said in another e-mail:
https://www.postgresql.org/message-id/flat/20240110.120644.1876591646729327180.kou%40clear-code.com#d923173e9625c20319750155083cbd72

>> 2. Need an opaque space like IndexScanDesc::opaque does
>>
>> * A custom COPY TO handler needs to keep its data
>
> I once thought users might want to parse their own options, maybe this
> is a use case for this opaque space.

Good catch! I forgot to suggest a callback for custom format
options. How about the following API?

----
...
typedef bool (*CopyToProcessOption_function) (CopyToState cstate, DefElem *defel);

...
typedef bool (*CopyFromProcessOption_function) (CopyFromState cstate, DefElem *defel);

typedef struct CopyToFormatRoutine
{
...
CopyToProcessOption_function process_option_fn;
} CopyToFormatRoutine;

typedef struct CopyFromFormatRoutine
{
...
CopyFromProcessOption_function process_option_fn;
} CopyFromFormatRoutine;
----

----
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e7597894bf..1aa8b62551 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -416,6 +416,7 @@ void
ProcessCopyOptions(ParseState *pstate,
CopyFormatOptions *opts_out,
bool is_from,
+ void *cstate, /* CopyToState* for !is_from, CopyFromState* for is_from */
List *options)
{
bool format_specified = false;
@@ -593,11 +594,19 @@ ProcessCopyOptions(ParseState *pstate,
parser_errposition(pstate, defel->location)));
}
else
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("option \"%s\" not recognized",
- defel->defname),
- parser_errposition(pstate, defel->location)));
+ {
+ bool processed;
+ if (is_from)
+ processed = opts_out->from_ops->process_option_fn(cstate, defel);
+ else
+ processed = opts_out->to_ops->process_option_fn(cstate, defel);
+ if (!processed)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" not recognized",
+ defel->defname),
+ parser_errposition(pstate, defel->location)));
+ }
}

/*
----

> For the name, I thought private_data might be a better candidate than
> opaque, but I do not insist.

I don't have a strong opinion for this. Here are the number
of headers that use "private_data" and "opaque":

$ grep -r private_data --files-with-matches src/include | wc -l
6
$ grep -r opaque --files-with-matches src/include | wc -l
38

It seems that we use "opaque" than "private_data" in general.

but it seems that we use
"opaque" than "private_data" in our code.

> Do you use the arrow library to control the memory?

Yes.

> Is there a way that
> we can let the arrow use postgres' memory context?

Yes. Apache Arrow C++ provides a memory pool feature and we
can implement PostgreSQL's memory context based memory pool
for this. (But this is a custom COPY TO/FROM handler's
implementation details.)

> I'm not sure this
> is necessary, just raise the question for discussion.

Could you clarify what should we discuss? We should require
that COPY TO/FROM handlers should use PostgreSQL's memory
context for all internal memory allocations?

> +PG_FUNCTION_INFO_V1(copy_testfmt_handler);
> +Datum
> +copy_testfmt_handler(PG_FUNCTION_ARGS)
> +{
> + bool is_from = PG_GETARG_BOOL(0);
> + CopyFormatRoutine *cp = makeNode(CopyFormatRoutine);;
> +
>
> extra semicolon.

I noticed it too :-)
But I ignored it because the current implementation is only
for discussion. We know that it may be dirty.

Thanks,
--
kou

On Wed, Jan 10, 2024 at 12:00 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <ZYTfqGppMc9e_w2k(at)paquier(dot)xyz>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Dec 2023 10:00:24 +0900,
> Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> >> 3. Export CopySend*()
> >>
> >> * If we like minimum API, we just need to export
> >> CopySendData() and CopySendEndOfRow(). But
> >> CopySend{String,Char,Int32,Int16}() will be convenient
> >> custom COPY TO handlers. (A custom COPY TO handler for
> >> Apache Arrow doesn't need them.)
> >
> > Hmm. Not sure on this one. This may come down to externalize the
> > manipulation of fe_msgbuf. Particularly, could it be possible that
> > some custom formats don't care at all about the network order?
>
> It means that all custom formats should control byte order
> by themselves instead of using CopySendInt*() that always
> use network byte order, right? It makes sense. Let's export
> only CopySendData() and CopySendEndOfRow().
>
>
> >> 1. What value should be used for "format" in
> >> PgMsg_CopyOutResponse message?
> >>
> >> It's 1 for binary format and 0 for text/csv format.
> >>
> >> Should we make it customizable by custom COPY TO handler?
> >> If so, what value should be used for this?
> >
> > Interesting point. It looks very tempting to give more flexibility to
> > people who'd like to use their own code as we have one byte in the
> > protocol but just use 0/1. Hence it feels natural to have a callback
> > for that.
>
> OK. Let's add a callback something like:
>
> typedef int16 (*CopyToGetFormat_function) (CopyToState cstate);
>
> > It also means that we may want to think harder about copy_is_binary in
> > libpq in the future step. Now, having a backend implementation does
> > not need any libpq bits, either, because a client stack may just want
> > to speak the Postgres protocol directly. Perhaps a custom COPY
> > implementation would be OK with how things are in libpq, as well,
> > tweaking its way through with just text or binary.
>
> Can we defer this discussion after we commit a basic custom
> COPY format handler mechanism?
>
> >> 2. Do we need more tries for design discussion for the first
> >> implementation? If we need, what should we try?
> >
> > A makeNode() is used with an allocation in the current memory context
> > in the function returning the handler. I would have assume that this
> > stuff returns a handler as a const struct like table AMs.
>
> If we use this approach, we can't use the Sawada-san's
> idea[1] that provides a convenient API to hide
> CopyFormatRoutine internal. The idea provides
> MakeCopy{To,From}FormatRoutine(). They return a new
> CopyFormatRoutine* with suitable is_from member. They can't
> use static const CopyFormatRoutine because they may be called
> multiple times in the same process.
>
> We can use the satic const struct approach by choosing one
> of the followings:
>
> 1. Use separated function for COPY {TO,FROM} format handlers
> as I suggested.
>
> 2. Don't provide convenient API. Developers construct
> CopyFormatRoutine by themselves. But it may be a bit
> tricky.
>
> 3. Similar to 2. but don't use a bit tricky approach (don't
> embed Copy{To,From}FormatRoutine nodes into
> CopyFormatRoutine).
>
> Use unified function for COPY {TO,FROM} format handlers
> but CopyFormatRoutine always have both of COPY {TO,FROM}
> format routines and these routines aren't nodes:
>
> typedef struct CopyToFormatRoutine
> {
> CopyToStart_function start_fn;
> CopyToOneRow_function onerow_fn;
> CopyToEnd_function end_fn;
> } CopyToFormatRoutine;
>
> /* XXX: just copied from COPY TO routines */
> typedef struct CopyFromFormatRoutine
> {
> CopyFromStart_function start_fn;
> CopyFromOneRow_function onerow_fn;
> CopyFromEnd_function end_fn;
> } CopyFromFormatRoutine;
>
> typedef struct CopyFormatRoutine
> {
> NodeTag type;
>
> CopyToFormatRoutine to_routine;
> CopyFromFormatRoutine from_routine;
> } CopyFormatRoutine;
>
> ----
>
> static const CopyFormatRoutine testfmt_handler = {
> .type = T_CopyFormatRoutine,
> .to_routine = {
> .start_fn = testfmt_copyto_start,
> .onerow_fn = testfmt_copyto_onerow,
> .end_fn = testfmt_copyto_end,
> },
> .from_routine = {
> .start_fn = testfmt_copyfrom_start,
> .onerow_fn = testfmt_copyfrom_onerow,
> .end_fn = testfmt_copyfrom_end,
> },
> };
>
> PG_FUNCTION_INFO_V1(copy_testfmt_handler);
> Datum
> copy_testfmt_handler(PG_FUNCTION_ARGS)
> {
> PG_RETURN_POINTER(&testfmt_handler);
> }
>
> 4. ... other idea?

It's a just idea but the fourth idea is to provide a convenient macro
to make it easy to construct the CopyFormatRoutine. For example,

#define COPYTO_ROUTINE(...) (Node *) &(CopyToFormatRoutine) {__VA_ARGS__}

static const CopyFormatRoutine testfmt_copyto_handler = {
.type = T_CopyFormatRoutine,
.is_from = true,
.routine = COPYTO_ROUTINE (
.start_fn = testfmt_copyto_start,
.onerow_fn = testfmt_copyto_onerow,
.end_fn = testfmt_copyto_end
)
};

Datum
copy_testfmt_handler(PG_FUNCTION_ARGS)
{
PG_RETURN_POINTER(& testfmt_copyto_handler);
}

Regards,

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

Hi,

In <CAD21AoC_dhfS97DKwTL+2nvgBOYrmN9XVYrE8w2SuDgghb-yzg(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Jan 2024 15:33:22 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

>> We can use the satic const struct approach by choosing one
>> of the followings:
>>
>> ...
>>
>> 4. ... other idea?
>
> It's a just idea but the fourth idea is to provide a convenient macro
> to make it easy to construct the CopyFormatRoutine. For example,
>
> #define COPYTO_ROUTINE(...) (Node *) &(CopyToFormatRoutine) {__VA_ARGS__}
>
> static const CopyFormatRoutine testfmt_copyto_handler = {
> .type = T_CopyFormatRoutine,
> .is_from = true,
> .routine = COPYTO_ROUTINE (
> .start_fn = testfmt_copyto_start,
> .onerow_fn = testfmt_copyto_onerow,
> .end_fn = testfmt_copyto_end
> )
> };
>
> Datum
> copy_testfmt_handler(PG_FUNCTION_ARGS)
> {
> PG_RETURN_POINTER(& testfmt_copyto_handler);
> }

Interesting. But I feel that it introduces another (a bit)
tricky mechanism...

BTW, we also need to set .type:

.routine = COPYTO_ROUTINE (
.type = T_CopyToFormatRoutine,
.start_fn = testfmt_copyto_start,
.onerow_fn = testfmt_copyto_onerow,
.end_fn = testfmt_copyto_end
)

Thanks,
--
kou

On Wed, Jan 10, 2024 at 3:40 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoC_dhfS97DKwTL+2nvgBOYrmN9XVYrE8w2SuDgghb-yzg(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Jan 2024 15:33:22 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> >> We can use the satic const struct approach by choosing one
> >> of the followings:
> >>
> >> ...
> >>
> >> 4. ... other idea?
> >
> > It's a just idea but the fourth idea is to provide a convenient macro
> > to make it easy to construct the CopyFormatRoutine. For example,
> >
> > #define COPYTO_ROUTINE(...) (Node *) &(CopyToFormatRoutine) {__VA_ARGS__}
> >
> > static const CopyFormatRoutine testfmt_copyto_handler = {
> > .type = T_CopyFormatRoutine,
> > .is_from = true,
> > .routine = COPYTO_ROUTINE (
> > .start_fn = testfmt_copyto_start,
> > .onerow_fn = testfmt_copyto_onerow,
> > .end_fn = testfmt_copyto_end
> > )
> > };
> >
> > Datum
> > copy_testfmt_handler(PG_FUNCTION_ARGS)
> > {
> > PG_RETURN_POINTER(& testfmt_copyto_handler);
> > }
>
> Interesting. But I feel that it introduces another (a bit)
> tricky mechanism...

Right. On the other hand, I don't think the idea 3 is good for the
same reason Michael-san pointed out before[1][2].

>
> BTW, we also need to set .type:
>
> .routine = COPYTO_ROUTINE (
> .type = T_CopyToFormatRoutine,
> .start_fn = testfmt_copyto_start,
> .onerow_fn = testfmt_copyto_onerow,
> .end_fn = testfmt_copyto_end
> )

I think it's fine as the same is true for table AM.

[1] https://www.postgresql.org/message-id/ZXEUIy6wl4jHy6Nm%40paquier.xyz
[2] https://www.postgresql.org/message-id/ZXKm9tmnSPIVrqZz%40paquier.xyz

Regards,

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

Hi,

In <CAD21AoC4HVuxOrsX1fLwj=5hdEmjvZoQw6PJGzxqxHNnYSQUVQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Jan 2024 16:53:48 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

>> Interesting. But I feel that it introduces another (a bit)
>> tricky mechanism...
>
> Right. On the other hand, I don't think the idea 3 is good for the
> same reason Michael-san pointed out before[1][2].
>
> [1] https://www.postgresql.org/message-id/ZXEUIy6wl4jHy6Nm%40paquier.xyz
> [2] https://www.postgresql.org/message-id/ZXKm9tmnSPIVrqZz%40paquier.xyz

I think that the important part of the Michael-san's opinion
is "keep COPY TO implementation and COPY FROM implementation
separated for maintainability".

The patch focused in [1][2] uses one routine for both of
COPY TO and COPY FROM. If we use the approach, we need to
change one common routine from copyto.c and copyfrom.c (or
export callbacks from copyto.c and copyfrom.c and use them
in copyto.c to construct one common routine). It's
the problem.

The idea 3 still has separated routines for COPY TO and COPY
FROM. So I think that it still keeps COPY TO implementation
and COPY FROM implementation separated.

>> BTW, we also need to set .type:
>>
>> .routine = COPYTO_ROUTINE (
>> .type = T_CopyToFormatRoutine,
>> .start_fn = testfmt_copyto_start,
>> .onerow_fn = testfmt_copyto_onerow,
>> .end_fn = testfmt_copyto_end
>> )
>
> I think it's fine as the same is true for table AM.

Ah, sorry. I should have said explicitly. I don't this that
it's not a problem. I just wanted to say that it's missing.

Defining one more static const struct instead of providing a
convenient (but a bit tricky) macro may be straightforward:

static const CopyToFormatRoutine testfmt_copyto_routine = {
.type = T_CopyToFormatRoutine,
.start_fn = testfmt_copyto_start,
.onerow_fn = testfmt_copyto_onerow,
.end_fn = testfmt_copyto_end
};

static const CopyFormatRoutine testfmt_copyto_handler = {
.type = T_CopyFormatRoutine,
.is_from = false,
.routine = (Node *) &testfmt_copyto_routine
};

Thanks,
--
kou

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

Hi,

On Wed, Jan 10, 2024 at 2:20 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3+jG_NKOUmcxDyEX2xSggBXReZ4H=e3RFsUtedY88A03w(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Dec 2023 10:58:05 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> >> 1. Add an opaque space for custom COPY TO handler
> >> * Add CopyToState{Get,Set}Opaque()
> >> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
> >>
> >> 2. Export CopyToState::attnumlist
> >> * Add CopyToStateGetAttNumList()
> >> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688
> >>
> >> 3. Export CopySend*()
> >> * Rename CopySend*() to CopyToStateSend*() and export them
> >> * Exception: CopySendEndOfRow() to CopyToStateFlush() because
> >> it just flushes the internal buffer now.
> >> https://github.com/kou/postgres/commit/289a5640135bde6733a1b8e2c412221ad522901e
> >>
> > I guess the purpose of these helpers is to avoid expose CopyToState to
> > copy.h,
>
> Yes.
>
> > but I
> > think expose CopyToState to user might make life easier, users might want to use
> > the memory contexts of the structure (though I agree not all the
> > fields are necessary
> > for extension handers).
>
> OK. I don't object it as I said in another e-mail:
> https://www.postgresql.org/message-id/flat/20240110.120644.1876591646729327180.kou%40clear-code.com#d923173e9625c20319750155083cbd72
>
> >> 2. Need an opaque space like IndexScanDesc::opaque does
> >>
> >> * A custom COPY TO handler needs to keep its data
> >
> > I once thought users might want to parse their own options, maybe this
> > is a use case for this opaque space.
>
> Good catch! I forgot to suggest a callback for custom format
> options. How about the following API?
>
> ----
> ...
> typedef bool (*CopyToProcessOption_function) (CopyToState cstate, DefElem *defel);
>
> ...
> typedef bool (*CopyFromProcessOption_function) (CopyFromState cstate, DefElem *defel);
>
> typedef struct CopyToFormatRoutine
> {
> ...
> CopyToProcessOption_function process_option_fn;
> } CopyToFormatRoutine;
>
> typedef struct CopyFromFormatRoutine
> {
> ...
> CopyFromProcessOption_function process_option_fn;
> } CopyFromFormatRoutine;
> ----
>
> ----
> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
> index e7597894bf..1aa8b62551 100644
> --- a/src/backend/commands/copy.c
> +++ b/src/backend/commands/copy.c
> @@ -416,6 +416,7 @@ void
> ProcessCopyOptions(ParseState *pstate,
> CopyFormatOptions *opts_out,
> bool is_from,
> + void *cstate, /* CopyToState* for !is_from, CopyFromState* for is_from */
> List *options)
> {
> bool format_specified = false;
> @@ -593,11 +594,19 @@ ProcessCopyOptions(ParseState *pstate,
> parser_errposition(pstate, defel->location)));
> }
> else
> - ereport(ERROR,
> - (errcode(ERRCODE_SYNTAX_ERROR),
> - errmsg("option \"%s\" not recognized",
> - defel->defname),
> - parser_errposition(pstate, defel->location)));
> + {
> + bool processed;
> + if (is_from)
> + processed = opts_out->from_ops->process_option_fn(cstate, defel);
> + else
> + processed = opts_out->to_ops->process_option_fn(cstate, defel);
> + if (!processed)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("option \"%s\" not recognized",
> + defel->defname),
> + parser_errposition(pstate, defel->location)));
> + }
> }
>
> /*
> ----

Looks good.

>
> > For the name, I thought private_data might be a better candidate than
> > opaque, but I do not insist.
>
> I don't have a strong opinion for this. Here are the number
> of headers that use "private_data" and "opaque":
>
> $ grep -r private_data --files-with-matches src/include | wc -l
> 6
> $ grep -r opaque --files-with-matches src/include | wc -l
> 38
>
> It seems that we use "opaque" than "private_data" in general.
>
> but it seems that we use
> "opaque" than "private_data" in our code.
>
> > Do you use the arrow library to control the memory?
>
> Yes.
>
> > Is there a way that
> > we can let the arrow use postgres' memory context?
>
> Yes. Apache Arrow C++ provides a memory pool feature and we
> can implement PostgreSQL's memory context based memory pool
> for this. (But this is a custom COPY TO/FROM handler's
> implementation details.)
>
> > I'm not sure this
> > is necessary, just raise the question for discussion.
>
> Could you clarify what should we discuss? We should require
> that COPY TO/FROM handlers should use PostgreSQL's memory
> context for all internal memory allocations?

Yes, handlers should use PostgreSQL's memory context, and I think
creating other memory context under CopyToStateData.copycontext
should be suggested for handler creators, so I proposed exporting
CopyToStateData to public header.
>
> > +PG_FUNCTION_INFO_V1(copy_testfmt_handler);
> > +Datum
> > +copy_testfmt_handler(PG_FUNCTION_ARGS)
> > +{
> > + bool is_from = PG_GETARG_BOOL(0);
> > + CopyFormatRoutine *cp = makeNode(CopyFormatRoutine);;
> > +
> >
> > extra semicolon.
>
> I noticed it too :-)
> But I ignored it because the current implementation is only
> for discussion. We know that it may be dirty.
>
>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

Hi,

In <CAEG8a3J02NzGBxG1rP9C4u7qRLOqUjSOdy3q5_5v__fydS3XcA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 12 Jan 2024 14:40:41 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

>> Could you clarify what should we discuss? We should require
>> that COPY TO/FROM handlers should use PostgreSQL's memory
>> context for all internal memory allocations?
>
> Yes, handlers should use PostgreSQL's memory context, and I think
> creating other memory context under CopyToStateData.copycontext
> should be suggested for handler creators, so I proposed exporting
> CopyToStateData to public header.

I see.

We can provide a getter for CopyToStateData::copycontext if
we don't want to export CopyToStateData. Note that I don't
have a strong opinion whether we should export
CopyToStateData or not.

Thanks,
--
kou

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 <20240112(dot)144615(dot)157925223373344229(dot)kou(at)clear-code(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 12 Jan 2024 14:46:15 +0900 (JST),
Sutou Kouhei <kou(at)clear-code(dot)com> 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
>
>

On Thu, Jan 11, 2024 at 10:24 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoC4HVuxOrsX1fLwj=5hdEmjvZoQw6PJGzxqxHNnYSQUVQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Jan 2024 16:53:48 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> >> Interesting. But I feel that it introduces another (a bit)
> >> tricky mechanism...
> >
> > Right. On the other hand, I don't think the idea 3 is good for the
> > same reason Michael-san pointed out before[1][2].
> >
> > [1] https://www.postgresql.org/message-id/ZXEUIy6wl4jHy6Nm%40paquier.xyz
> > [2] https://www.postgresql.org/message-id/ZXKm9tmnSPIVrqZz%40paquier.xyz
>
> I think that the important part of the Michael-san's opinion
> is "keep COPY TO implementation and COPY FROM implementation
> separated for maintainability".
>
> The patch focused in [1][2] uses one routine for both of
> COPY TO and COPY FROM. If we use the approach, we need to
> change one common routine from copyto.c and copyfrom.c (or
> export callbacks from copyto.c and copyfrom.c and use them
> in copyto.c to construct one common routine). It's
> the problem.
>
> The idea 3 still has separated routines for COPY TO and COPY
> FROM. So I think that it still keeps COPY TO implementation
> and COPY FROM implementation separated.
>
> >> BTW, we also need to set .type:
> >>
> >> .routine = COPYTO_ROUTINE (
> >> .type = T_CopyToFormatRoutine,
> >> .start_fn = testfmt_copyto_start,
> >> .onerow_fn = testfmt_copyto_onerow,
> >> .end_fn = testfmt_copyto_end
> >> )
> >
> > I think it's fine as the same is true for table AM.
>
> Ah, sorry. I should have said explicitly. I don't this that
> it's not a problem. I just wanted to say that it's missing.

Thank you for pointing it out.

>
>
> Defining one more static const struct instead of providing a
> convenient (but a bit tricky) macro may be straightforward:
>
> static const CopyToFormatRoutine testfmt_copyto_routine = {
> .type = T_CopyToFormatRoutine,
> .start_fn = testfmt_copyto_start,
> .onerow_fn = testfmt_copyto_onerow,
> .end_fn = testfmt_copyto_end
> };
>
> static const CopyFormatRoutine testfmt_copyto_handler = {
> .type = T_CopyFormatRoutine,
> .is_from = false,
> .routine = (Node *) &testfmt_copyto_routine
> };

Yeah, IIUC this is the option 2 you mentioned[1]. I think we can go
with this idea as it's the simplest. If we find a better way, we can
change it later. That is CopyFormatRoutine will be like:

typedef struct CopyFormatRoutine
{
NodeTag type;

/* either CopyToFormatRoutine or CopyFromFormatRoutine */
Node *routine;
} CopyFormatRoutine;

And the core can check the node type of the 'routine7 in the
CopyFormatRoutine returned by extensions.

Regards,

[1] https://www.postgresql.org/message-id/20240110.120034.501385498034538233.kou%40clear-code.com

Regards,

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

Hi,

In <CAD21AoB5x86TTyer90iSFivnSD8MFRU8V4ALzmQ=rQFw4QqiXQ(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 15 Jan 2024 16:03:41 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

>> Defining one more static const struct instead of providing a
>> convenient (but a bit tricky) macro may be straightforward:
>>
>> static const CopyToFormatRoutine testfmt_copyto_routine = {
>> .type = T_CopyToFormatRoutine,
>> .start_fn = testfmt_copyto_start,
>> .onerow_fn = testfmt_copyto_onerow,
>> .end_fn = testfmt_copyto_end
>> };
>>
>> static const CopyFormatRoutine testfmt_copyto_handler = {
>> .type = T_CopyFormatRoutine,
>> .is_from = false,
>> .routine = (Node *) &testfmt_copyto_routine
>> };
>
> Yeah, IIUC this is the option 2 you mentioned[1]. I think we can go
> with this idea as it's the simplest.
>
> [1] https://www.postgresql.org/message-id/20240110.120034.501385498034538233.kou%40clear-code.com

Ah, you're right. I forgot it...

> That is CopyFormatRoutine will be like:
>
> typedef struct CopyFormatRoutine
> {
> NodeTag type;
>
> /* either CopyToFormatRoutine or CopyFromFormatRoutine */
> Node *routine;
> } CopyFormatRoutine;
>
> And the core can check the node type of the 'routine7 in the
> CopyFormatRoutine returned by extensions.

It makes sense.

If no more comments about the current design, I'll start
implementing this feature based on the current design.

Thanks,
--
kou

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/CAD21AoCunywHird3GaPzWe6s9JG1wzxj3Cr6vGN36DDheGjOjA@mail.gmail.com

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 <20240115(dot)152702(dot)2011620917962812379(dot)kou(at)clear-code(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 15 Jan 2024 15:27:02 +0900 (JST),
Sutou Kouhei <kou(at)clear-code(dot)com> 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 <20240112(dot)144615(dot)157925223373344229(dot)kou(at)clear-code(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 12 Jan 2024 14:46:15 +0900 (JST),
> Sutou Kouhei <kou(at)clear-code(dot)com> 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
>>
>>
>
>

On Wed, Jan 24, 2024 at 02:49:36PM +0900, Sutou Kouhei wrote:
> For COPY TO:
>
> 0001: This adds CopyToRoutine and use it for text/csv/binary
> 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

On 2024-01-24 We 03:11, Michael Paquier wrote:
> On Wed, Jan 24, 2024 at 02:49:36PM +0900, Sutou Kouhei wrote:
>> For COPY TO:
>>
>> 0001: This adds CopyToRoutine and use it for text/csv/binary
>> 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.

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

Hi,

In <10025bac-158c-ffe7-fbec-32b42629121f(at)dunslane(dot)net>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 24 Jan 2024 07:15:55 -0500,
Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>
> On 2024-01-24 We 03:11, Michael Paquier wrote:
>> On Wed, Jan 24, 2024 at 02:49:36PM +0900, Sutou Kouhei wrote:
>>> For COPY TO:
>>>
>>> 0001: This adds CopyToRoutine and use it for text/csv/binary
>>> 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.
>
>
> 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.

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

Hi,

In <20240124(dot)144936(dot)67229716500876806(dot)kou(at)clear-code(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 24 Jan 2024 14:49:36 +0900 (JST),
Sutou Kouhei <kou(at)clear-code(dot)com> wrote:

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

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

On Wed, Jan 24, 2024 at 10:17 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> 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?
>

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.

On Wed, Jan 24, 2024 at 11:17:26PM +0900, Sutou Kouhei wrote:
> In <10025bac-158c-ffe7-fbec-32b42629121f(at)dunslane(dot)net>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 24 Jan 2024 07:15:55 -0500,
> Andrew Dunstan <andrew(at)dunslane(dot)net> 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.

Clearly.

> I uploaded my benchmark script so that you can run the same
> benchmark on your machine:
>
> 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

On Thu, Jan 25, 2024 at 10:53:58AM +0800, jian he wrote:
> 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.

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

On Wed, Jan 24, 2024 at 11:17 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <10025bac-158c-ffe7-fbec-32b42629121f(at)dunslane(dot)net>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 24 Jan 2024 07:15:55 -0500,
> Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> >
> > On 2024-01-24 We 03:11, Michael Paquier wrote:
> >> On Wed, Jan 24, 2024 at 02:49:36PM +0900, Sutou Kouhei wrote:
> >>> For COPY TO:
> >>>
> >>> 0001: This adds CopyToRoutine and use it for text/csv/binary
> >>> 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.
> >
> >
> > 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.
>
> 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?
>

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

On Thu, Jan 25, 2024 at 01:36:03PM +0900, Masahiko Sawada wrote:
> 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.

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

On Thu, Jan 25, 2024 at 1:53 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Jan 25, 2024 at 01:36:03PM +0900, Masahiko Sawada wrote:
> > 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.
>
> 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.

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

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

Probably. Seems worth evaluating.

Regards,

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

Hi,

Thanks for trying these patches!

In <CACJufxF9NS3xQ2d79jN0V1CGvF7cR16uJo-C3nrY7vZrwvxF7w(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 10:53:58 +0800,
jian he <jian(dot)universality(at)gmail(dot)com> wrote:

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

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

> Time: 668.996 ms
> Time: 596.254 ms
> Time: 592.723 ms
> Time: 591.663 ms
> Time: 590.803 ms

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

Thanks,
--
kou

Hi,

In <ZbHS439y-Bs6HIAR(at)paquier(dot)xyz>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 12:17:55 +0900,
Michael Paquier <michael(at)paquier(dot)xyz> wrote:

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

OK. I'll do it.

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

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

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

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

I'll use the following style:

/*
* ...
*/

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

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
> split the assignment of cstate->out_functions into its own callback?

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

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

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

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

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

Hi,

In <CAD21AoALxEZz33NpcSk99ad_DT3A2oFNMa2KNjGBCMVFeCiUaA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 13:36:03 +0900,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

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

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

On Thu, Jan 25, 2024 at 05:45:43PM +0900, Sutou Kouhei wrote:
> In <ZbHS439y-Bs6HIAR(at)paquier(dot)xyz>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 12:17:55 +0900,
> Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> +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().
>
> OK. How about the following for the fetch function
> signature?
>
> 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:
>
> typedef enum CopyBuiltinFormat
> {
> COPY_BUILTIN_FORMAT_TEXT = 0,
> COPY_BUILTIN_FORMAT_CSV,
> COPY_BUILTIN_FORMAT_BINARY,
> } CopyBuiltinFormat;
>
> 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]);
>>
>> 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?
>
> 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?
>
> 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.
>
> 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.
>
> 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

On Thu, Jan 25, 2024 at 4:52 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoALxEZz33NpcSk99ad_DT3A2oFNMa2KNjGBCMVFeCiUaA(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 25 Jan 2024 13:36:03 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > 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.
>
> 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

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

Hi,

In <CAEG8a3+-oG63GeG6v0L8EWi_8Fhuj9vJBhOteLxuBZwtun3GVA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 16:18:14 +0800,
Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:

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

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

On Fri, Jan 26, 2024 at 4:32 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3+-oG63GeG6v0L8EWi_8Fhuj9vJBhOteLxuBZwtun3GVA(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 16:18:14 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> > 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)?
>
> 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?

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.

>
>
> Thanks,
> --
> kou

--
Regards
Junwang Zhao

Hi,

In <ZbLwNyOKbddno0Ue(at)paquier(dot)xyz>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 08:35:19 +0900,
Michael Paquier <michael(at)paquier(dot)xyz> wrote:

>> OK. How about the following for the fetch function
>> signature?
>>
>> extern CopyToRoutine *GetBuiltinCopyToRoutine(const char *format);
>
> Or CopyToRoutineGet()? I am not wedded to my suggestion, got a bad
> 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?

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

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