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

On Fri, Feb 28, 2025 at 3:06 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> 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.
>

Thank you for the confirmation. Pushed the fix.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com