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

On Thu, Jan 11, 2024 at 10:24 AM Sutou Kouhei <kou@clear-code.com> wrote:
>
> Hi,
>
> In <CAD21AoC4HVuxOrsX1fLwj=5hdEmjvZoQw6PJGzxqxHNnYSQUVQ@mail.gmail.com>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Jan 2024 16:53:48 +0900,
>   Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> >> Interesting. But I feel that it introduces another (a bit)
> >> tricky mechanism...
> >
> > Right. On the other hand, I don't think the idea 3 is good for the
> > same reason Michael-san pointed out before[1][2].
> >
> > [1] https://www.postgresql.org/message-id/ZXEUIy6wl4jHy6Nm%40paquier.xyz
> > [2] https://www.postgresql.org/message-id/ZXKm9tmnSPIVrqZz%40paquier.xyz
>
> I think that the important part of the Michael-san's opinion
> is "keep COPY TO implementation and COPY FROM implementation
> separated for maintainability".
>
> The patch focused in [1][2] uses one routine for both of
> COPY TO and COPY FROM. If we use the approach, we need to
> change one common routine from copyto.c and copyfrom.c (or
> export callbacks from copyto.c and copyfrom.c and use them
> in copyto.c to construct one common routine). It's
> the problem.
>
> The idea 3 still has separated routines for COPY TO and COPY
> FROM. So I think that it still keeps COPY TO implementation
> and COPY FROM implementation separated.
>
> >> BTW, we also need to set .type:
> >>
> >>      .routine = COPYTO_ROUTINE (
> >>          .type = T_CopyToFormatRoutine,
> >>          .start_fn = testfmt_copyto_start,
> >>          .onerow_fn = testfmt_copyto_onerow,
> >>          .end_fn = testfmt_copyto_end
> >>          )
> >
> > I think it's fine as the same is true for table AM.
>
> Ah, sorry. I should have said explicitly. I don't this that
> it's not a problem. I just wanted to say that it's missing.

Thank you for pointing it out.

>
>
> Defining one more static const struct instead of providing a
> convenient (but a bit tricky) macro may be straightforward:
>
> static const CopyToFormatRoutine testfmt_copyto_routine = {
>     .type = T_CopyToFormatRoutine,
>     .start_fn = testfmt_copyto_start,
>     .onerow_fn = testfmt_copyto_onerow,
>     .end_fn = testfmt_copyto_end
> };
>
> static const CopyFormatRoutine testfmt_copyto_handler = {
>     .type = T_CopyFormatRoutine,
>     .is_from = false,
>     .routine = (Node *) &testfmt_copyto_routine
> };

Yeah, IIUC this is the option 2 you mentioned[1]. I think we can go
with this idea as it's the simplest. If we find a better way, we can
change it later. That is CopyFormatRoutine will be like:

typedef struct CopyFormatRoutine
{
    NodeTag     type;

    /* either CopyToFormatRoutine or CopyFromFormatRoutine */
    Node       *routine;
}           CopyFormatRoutine;

And the core can check the node type of the 'routine7 in the
CopyFormatRoutine returned by extensions.

Regards,

[1] https://www.postgresql.org/message-id/20240110.120034.501385498034538233.kou%40clear-code.com

Regards,

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