Date: Thu, 11 Jan 2024 10:24:45 +0900 (JST)
Message-Id: <20240111.102445.1008569098168542133.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, zhjwpku@gmail.com, andrew@dunslane.net,
 nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: 
 <CAD21AoC4HVuxOrsX1fLwj=5hdEmjvZoQw6PJGzxqxHNnYSQUVQ@mail.gmail.com>
References: 
 <CAD21AoC_dhfS97DKwTL+2nvgBOYrmN9XVYrE8w2SuDgghb-yzg@mail.gmail.com>
	<20240110.154028.639233626061692638.kou@clear-code.com>
	<CAD21AoC4HVuxOrsX1fLwj=5hdEmjvZoQw6PJGzxqxHNnYSQUVQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240111.102445.1008569098168542133.kou%40clear-code.com>
Precedence: bulk

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.


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
};


Thanks,
-- 
kou