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

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'Junwang Zhao' <zhjwpku(at)gmail(dot)com>, Sutou Kouhei <kou(at)clear-code(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "nathandbossart(at)gmail(dot)com" <nathandbossart(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: Make COPY format extendable: Extract COPY TO format implementations
Date: 2023-12-09 02:43:49
Message-ID: TY3PR01MB9889C9234CD220A3A7075F0DF589A@TY3PR01MB9889.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Lakhin 2023-12-09 04:00:01 How abnormal server shutdown could be detected by tests?
Previous Message Peter Geoghegan 2023-12-09 02:29:17 Re: Bug in nbtree optimization to skip > operator comparisons (or < comparisons in backwards scans)