On 2025-02-28 Fr 5:39 PM, Masahiko Sawada wrote:
On Fri, Feb 28, 2025 at 2:16 PM Sutou Kouhei<k...@clear-code.com> wrote:
Hi,
Thanks for following up the patch!
In<cad21aoba414q76lthy65njfwbjoxxn1bdffsd_nbht2wpus...@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.m...@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
