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?
The framework in the patch LGTM.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
--
Regards
Junwang Zhao