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

On Mon, Feb 05, 2024 at 06:05:15PM +0900, Sutou Kouhei wrote:
> In <ZcCKwAeFrlOqPBuN@paquier.xyz>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 5 Feb 2024 16:14:08 +0900,
>   Michael Paquier <michael@paquier.xyz> wrote:
>> 2) I have backpedaled on the postpare callback, which did not bring
>> much in clarity IMO while being a CSV-only callback.  Note that we
>> have in copyfromparse.c more paths that are only for CSV but the past
>> versions of the patch never cared about that.  This makes the text and
>> CSV implementations much closer to each other, as a result.
>
> Ah, sorry. I forgot to eliminate cstate->opts.csv_mode in
> CopyReadLineText(). The postpare callback is for
> optimization. If it doesn't improve performance, we don't
> need to introduce it.

No worries.

> We may want to try eliminating cstate->opts.csv_mode in
> CopyReadLineText() for performance. But we don't need to
> do this in introducing CopyFromRoutine. We can defer it.
>
> So I don't object removing the postpare callback.

Rather related, but there has been a comment from Andres about this
kind of splits a few hours ago, so perhaps this is for the best:
https://www.postgresql.org/message-id/20240205182118.h5rkbnjgujwzuxip%40awork3.anarazel.de

I'll reply to this one in a bit.

>>                                              Let me know if you have
>> comments about all that.
>
> Here are some comments for the patch:

Thanks.  My head was spinning after reading the diffs more than 20
times :)

> fmgr_info ->
> finfo
> optinally ->
> optionally
> CopyFromRoutine->OneRow ->
> CopyFromRoutine->CopyFromOneRow
> CopyFromTextStart ->
> CopyFromBinaryStart
> CopyFromTextEnd ->
> CopyFromBinaryEnd

Fixed all these.

> How about passing CopyFromState cstate too like other
> callbacks for consistency?

Yes, I was wondering a bit if this can be useful for the custom
formats.

> +    /*
> +     * Copy one row to a set of `values` and `nulls` of size tupDesc->natts.
> +     *
> +     * 'econtext' is used to evaluate default expression for each column that
> +     * is either not read from the file or is using the DEFAULT option of COPY
>
> or is ->
> or

"or is" is correct here IMO.

> Wow! I didn't know that we need to update typedefs.list when
> I add a "typedef struct".

That's for the automated indentation.  This is a habit I have when it
comes to work on shaping up patches to avoid weird diffs with pgindent
and new structure names.  It's OK to forget about it :)

Attaching a v13 for now.
--
Michael