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

On Mon, Jan 29, 2024 at 2:03 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAEG8a3JDPks7XU5-NvzjzuKQYQqR8pDfS7CDGZonQTXfdWtnnw(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 27 Jan 2024 14:15:02 +0800,
> Junwang Zhao <zhjwpku(at)gmail(dot)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