Re: pgsql: Refactor COPY FROM to use format callback functions.

On 2025-02-28 Fr 5:39 PM, Masahiko Sawada wrote:
> On Fri, Feb 28, 2025 at 2:16 PM Sutou Kouhei<kou(at)clear-code(dot)com> wrote:
>> Hi,
>>
>> Thanks for following up the patch!
>>
>> In<CAD21AoBA414Q76LthY65NJfWbjOxXn1bdFFsD_NBhT2wPUS1SQ(at)mail(dot)gmail(dot)com>
>> "Re: pgsql: Refactor COPY FROM to use format callback functions." on Fri, 28 Feb 2025 12:56:19 -0800,
>> Masahiko Sawada<sawada(dot)mshk(at)gmail(dot)com> wrote:
>>
>>> Right. I've attached the updated patch.
>> In general, this approach will work but could you keep
>> the same signature for backward compatibility?
>>
>>> --- a/src/backend/commands/copyfromparse.c
>>> +++ b/src/backend/commands/copyfromparse.c
>>> +bool
>>> +NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
>>> +{
>>> + return NextCopyFromRawFieldsInternal(cstate, fields, nfields, is_csv);
>>> +}
>> NextCopyFromRawFields() uses
>> NextCopyFromRawFields(CopyFromState cstate, char ***fields,
>> int *nfields) (no "bool is_csv") before we remove it. So
>> could you use the no "bool is_csv" signature for backward
>> compatibility?
>>
>> bool
>> NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
>> {
>> return NextCopyFromRawFieldsInternal(cstate, fields, nfields, cstate->opts.csv_mode);
>> }
> I like this idea.
>
>>
>>> @@ -738,6 +742,15 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
>>> /*
>>> * Read raw fields in the next line for COPY FROM in text or csv mode.
>>> * Return false if no more lines.
>>> + */
>>> +bool
>>> +NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
>>> +{
>>> + return NextCopyFromRawFieldsInternal(cstate, fields, nfields, is_csv);
>>> +}
>>> +
>>> +/*
>>> + * Workhorse for NextCopyFromRawFields().
>> It may be better that we don't split docs for
>> NextCopyFromRawFields() and
>> NextCopyFromRawFieldsInternal(). How about referring the
>> NextCopyFromRawFieldsInternal() doc from the
>> NextCopyFromRawFields() doc something like the following?
>>
>> /*
>> * See NextCopyFromRawFieldsInternal() for details.
>> */
>> bool
>> NextCopyFromRawFields(...)
>> {
>> ...
>> }
>>
>> /*
>> * Workhorse for NextCopyFromRawFields().
>> *
>> * Read raw fields ...
>> *
>> * ...
>> */
>> static pg_attribute_always_inline bool
>> NextCopyFromRawFieldsInternal()
>> {
>> ...
>> }
>>
> Agreed.
>
> I've updated the patch. I'm going to push it, barring any objections
> and further comments.
>

I'm OK either way - I have made changes to adapt to the API change, and
tested them.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com