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

On Fri, Feb 28, 2025 at 2:16 PM Sutou Kouhei <kou@clear-code.com> wrote:

Hi,

Thanks for following up the patch!

In <CAD21AoBA414Q76LthY65NJfWbjOxXn1bdFFsD_NBhT2wPUS1SQ@mail.gmail.com>  "Re: pgsql: Refactor COPY FROM to use format callback functions." on Fri, 28 Feb 2025 12:56:19 -0800,  Masahiko Sawada <sawada.mshk@gmail.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