RE: Make COPY format extendable: Extract COPY TO format implementations
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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
Commits
-
Refactor Copy{From|To}GetRoutine() to use pass-by-reference argument.
- bacbc4863b3b 18.0 landed
-
Refactor COPY FROM to use format callback functions.
- 7717f6300693 18.0 landed
-
Refactor COPY TO to use format callback functions.
- 2e4127b6d2d8 18.0 landed
-
Another try to fix BF failure introduced in commit ddd5f4f54a.
- 9bc1eee988c3 17.0 cited
-
Revert "Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM"
- 06bd311bce24 17.0 landed
-
Improve COPY TO performance when server and client encodings match
- b619852086ed 17.0 cited
-
Simplify signature of CopyAttributeOutCSV() in copyto.c
- b9d6038d7048 17.0 landed
-
Revert "Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO"
- 1aa8324b81fa 17.0 landed
-
Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO
- 2889fd23be56 17.0 landed
-
Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM
- 95fb5b49024a 17.0 landed
-
Add progress reporting of skipped tuples during COPY FROM.
- 729439607ad2 17.0 cited
-
pgbench: Add \syncpipeline
- 94edfe250c6a 17.0 cited
-
meson: Make gzip and tar optional
- 9ca6e7b9411e 17.0 cited
-
Export the external file reader used in COPY FROM as APIs.
- 8ddc05fb01ee 9.1.0 cited