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

From: Junwang Zhao <[email-masked]>
Date: Thu, 7 Dec 2023 16:37:36 +0800

On Thu, Dec 7, 2023 at 8:39 AM Michael Paquier <[email-masked]> 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

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 7 Dec 2023 11:38:47 -0500

> 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

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 8 Dec 2023 04:27:14 +0900

> the FROM handler, or a flag on each row indicating if it's a FROM or TO

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

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 8 Dec 2023 10:32:27 +0800

> > the FROM handler, or a flag on each row indicating if it's a FROM or TO

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.

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

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Sat, 9 Dec 2023 02:43:49 +0000

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

---

From: Sutou Kouhei <[email-masked]>
Date: Sun, 10 Dec 2023 05:44:07 +0900 (JST)

Hi,

Thanks for reviewing our latest patch!

In 
 <[email-masked]>
  "RE: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 9 Dec 2023 02:43:49 +0000,

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