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

On Mon, Jan 29, 2024 at 11:22 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jan 29, 2024 at 12:10 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > On Mon, Jan 29, 2024 at 10:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Jan 26, 2024 at 6:02 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > >
> > > > On Fri, Jan 26, 2024 at 4:55 PM Sutou Kouhei <kou@clear-code.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > In <CAEG8a3KhS6s1XQgDSvc8vFTb4GkhBmS8TxOoVSDPFX+MPExxxQ@mail.gmail.com>
> > > > >   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 26 Jan 2024 16:41:50 +0800,
> > > > >   Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > > >
> > > > > > CopyToProcessOption()/CopyFromProcessOption() can only handle
> > > > > > single option, and store the options in the opaque field,  but it can not
> > > > > > check the relation of two options, for example, considering json format,
> > > > > > the `header` option can not be handled by these two functions.
> > > > > >
> > > > > > I want to find a way when the user specifies the header option, customer
> > > > > > handler can error out.
> > > > >
> > > > > Ah, you want to use a built-in option (such as "header")
> > > > > value from a custom handler, right? Hmm, it may be better
> > > > > that we call CopyToProcessOption()/CopyFromProcessOption()
> > > > > for all options including built-in options.
> > > > >
> > > > Hmm, still I don't think it can handle all cases, since we don't know
> > > > the sequence of the options, we need all the options been parsed
> > > > before we check the compatibility of the options, or customer
> > > > handlers will need complicated logic to resolve that, which might
> > > > lead to ugly code :(
> > > >
> > >
> > > Does it make sense to pass only non-builtin options to the custom
> > > format callback after parsing and evaluating the builtin options? That
> > > is, we parse and evaluate only the builtin options and populate
> > > opts_out first, then pass each rest option to the custom format
> > > handler callback. The callback can refer to the builtin option values.
> >
> > Yeah, I think this makes sense.
> >
> > > The callback is expected to return false if the passed option is not
> > > supported. If one of the builtin formats is specified and the rest
> > > options list has at least one option, we raise "option %s not
> > > recognized" error.  IOW it's the core's responsibility to ranse the
> > > "option %s not recognized" error, which is in order to raise a
> > > consistent error message. Also, I think the core should check the
> > > redundant options including bultiin and custom options.
> >
> > It would be good that core could check all the redundant options,
> > but where should core do the book-keeping of all the options? I have
> > no idea about this, in my implementation of pg_copy_json extension,
> > I handle redundant options by adding a xxx_specified field for each
> > xxx.
>
> What I imagined is that while parsing the all specified options, we
> evaluate builtin options and we add non-builtin options to another
> list. Then when parsing a non-builtin option, we check if this option
> already exists in the list. If there is, we raise the "option %s not
> recognized" error.". Once we complete checking all options, we pass
> each option in the list to the callback.

LGTM.

>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com



-- 
Regards
Junwang Zhao