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

On Thu, Jan 11, 2024 at 10:24 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoC4HVuxOrsX1fLwj=5hdEmjvZoQw6PJGzxqxHNnYSQUVQ(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Jan 2024 16:53:48 +0900,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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