Horton

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

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Junwang Zhao <zhjwpku@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Sutou Kouhei <kou@clear-code.com>, andrew@dunslane.net, nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Date: 2023-12-13T11:48:18Z
Lists: pgsql-hackers 
On Tue, Dec 12, 2023 at 11:09 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Mon, Dec 11, 2023 at 10:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Dec 11, 2023 at 7:19 PM Michael Paquier <michael@paquier.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

Commits

  1. Refactor Copy{From|To}GetRoutine() to use pass-by-reference argument.

  2. Refactor COPY FROM to use format callback functions.

  3. Refactor COPY TO to use format callback functions.

  4. Another try to fix BF failure introduced in commit ddd5f4f54a.

  5. Revert "Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM"

  6. Improve COPY TO performance when server and client encodings match

  7. Simplify signature of CopyAttributeOutCSV() in copyto.c

  8. Revert "Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO"

  9. Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO

  10. Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM

  11. Add progress reporting of skipped tuples during COPY FROM.

  12. pgbench: Add \syncpipeline

  13. meson: Make gzip and tar optional

  14. Export the external file reader used in COPY FROM as APIs.