On Tue, Dec 12, 2023 at 11:09 AM Junwang Zhao <zhjw...@gmail.com> wrote: > > On Mon, Dec 11, 2023 at 10:32 PM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > On Mon, Dec 11, 2023 at 7:19 PM Michael Paquier <mich...@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
