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

On Mon, Jan 29, 2024 at 2:03 PM Sutou Kouhei <kou@clear-code.com> wrote:
>
> Hi,
>
> In <CAEG8a3JDPks7XU5-NvzjzuKQYQqR8pDfS7CDGZonQTXfdWtnnw@mail.gmail.com>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 27 Jan 2024 14:15:02 +0800,
>   Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> > I have been working on a *COPY TO JSON* extension since yesterday,
> > which is based on your V6 patch set, I'd like to give you more input
> > so you can make better decisions about the implementation(with only
> > pg-copy-arrow you might not get everything considered).
>
> Thanks!
>
> > 0009 is some changes made by me, I changed CopyToGetFormat to
> > CopyToSendCopyBegin because pg_copy_json need to send different bytes
> > in SendCopyBegin, get the format code along is not enough
>
> Oh, I haven't cared about the case.
> How about the following API instead?
>
> static void
> SendCopyBegin(CopyToState cstate)
> {
>         StringInfoData buf;
>
>         pq_beginmessage(&buf, PqMsg_CopyOutResponse);
>         cstate->opts.to_routine->CopyToFillCopyOutResponse(cstate, &buf);
>         pq_endmessage(&buf);
>         cstate->copy_dest = COPY_FRONTEND;
> }
>
> static void
> CopyToJsonFillCopyOutResponse(CopyToState cstate, StringInfoData &buf)
> {
>         int16           format = 0;
>
>         pq_sendbyte(&buf, format);      /* overall format */
>         /*
>          * JSON mode is always one non-binary column
>          */
>         pq_sendint16(&buf, 1);
>         pq_sendint16(&buf, format);
> }

Make sense to me.

>
> > 00010 is the pg_copy_json extension, I think this should be a good
> > case which can utilize the *extendable copy format* feature
>
> It seems that it's convenient that we have one more callback
> for initializing CopyToState::opaque. It's called only once
> when Copy{To,From}Routine is chosen:
>
> typedef struct CopyToRoutine
> {
>         void            (*CopyToInit) (CopyToState cstate);
> ...
> };

I like this, we can alloc private data in this hook.

>
> void
> ProcessCopyOptions(ParseState *pstate,
>                                    CopyFormatOptions *opts_out,
>                                    bool is_from,
>                                    void *cstate,
>                                    List *options)
> {
> ...
>         foreach(option, options)
>         {
>                 DefElem    *defel = lfirst_node(DefElem, option);
>
>                 if (strcmp(defel->defname, "format") == 0)
>                 {
>                         ...
>                         opts_out->to_routine = &CopyToRoutineXXX;
>                         opts_out->to_routine->CopyToInit(cstate);
>                         ...
>                 }
>         }
> ...
> }
>
>
> >                                                              maybe we
> > should delete copy_test_format if we have this extension as an
> > example?
>
> I haven't read the COPY TO format json thread[1] carefully
> (sorry), but we may add the JSON format as a built-in
> format. If we do it, copy_test_format is useful to test the
> extension API.

Yeah, maybe, I have no strong opinion here, pg_copy_json is
just a toy extension for discussion.

>
> [1] https://www.postgresql.org/message-id/flat/CALvfUkBxTYy5uWPFVwpk_7ii2zgT07t3d-yR_cy4sfrrLU%3Dkcg%40mail.gmail.com
>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao