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