pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Refactor COPY FROM to use format callback functions.
9+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2025-02-28 8a1012b35d Re-export NextCopyFromRawFields() to copy.h.
* pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 18:31 Masahiko Sawada <msawada@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2025-02-28 18:31 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Refactor COPY FROM to use format callback functions.
This commit introduces a new CopyFromRoutine struct, which is a set of
callback routines to read tuples in a specific format. It also makes
COPY FROM with the existing formats (text, CSV, and binary) utilize
these format callbacks.
This change is a preliminary step towards making the COPY FROM command
extensible in terms of input formats.
Similar to 2e4127b6d2d, this refactoring contributes to a performance
improvement by reducing the number of "if" branches that need to be
checked on a per-row basis when sending field representations in text
or CSV mode. The performance benchmark results showed ~5% performance
gain in text or CSV mode.
Author: Sutou Kouhei <kou@clear-code.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/20231204.153548.2126325458835528809.kou@clear-code.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/7717f63006935de00fafd000bff450280508adf1
Modified Files
--------------
src/backend/commands/copyfrom.c | 192 ++++++++++---
src/backend/commands/copyfromparse.c | 446 +++++++++++++++++--------------
src/include/commands/copy.h | 2 -
src/include/commands/copyapi.h | 50 +++-
src/include/commands/copyfrom_internal.h | 11 +
src/tools/pgindent/typedefs.list | 1 +
6 files changed, 459 insertions(+), 243 deletions(-)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 19:46 Andrew Dunstan <andrew@dunslane.net>
parent: Masahiko Sawada <msawada@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Dunstan @ 2025-02-28 19:46 UTC (permalink / raw)
To: Masahiko Sawada <msawada@postgresql.org>; pgsql-committers@lists.postgresql.org
On 2025-02-28 Fr 1:31 PM, Masahiko Sawada wrote:
> Refactor COPY FROM to use format callback functions.
>
> This commit introduces a new CopyFromRoutine struct, which is a set of
> callback routines to read tuples in a specific format. It also makes
> COPY FROM with the existing formats (text, CSV, and binary) utilize
> these format callbacks.
>
> This change is a preliminary step towards making the COPY FROM command
> extensible in terms of input formats.
>
> Similar to 2e4127b6d2d, this refactoring contributes to a performance
> improvement by reducing the number of "if" branches that need to be
> checked on a per-row basis when sending field representations in text
> or CSV mode. The performance benchmark results showed ~5% performance
> gain in text or CSV mode.
>
> Author: Sutou Kouhei<kou@clear-code.com>
> Reviewed-by: Masahiko Sawada<sawada.mshk@gmail.com>
> Reviewed-by: Michael Paquier<michael@paquier.xyz>
> Reviewed-by: Andres Freund<andres@anarazel.de>
> Reviewed-by: Tomas Vondra<tomas.vondra@enterprisedb.com>
> Reviewed-by: Junwang Zhao<zhjwpku@gmail.com>
> Discussion:https://postgr.es/m/20231204.153548.2126325458835528809.kou@clear-code.com
This patch has completely broken the file_textarray fdw, which uses
NextCopyFromRawFields(). Removing that from API is not a good thing.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 19:55 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2025-02-28 19:55 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Masahiko Sawada <msawada@postgresql.org>; pgsql-committers@lists.postgresql.org
On Fri, Feb 28, 2025 at 11:47 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-02-28 Fr 1:31 PM, Masahiko Sawada wrote:
>
> Refactor COPY FROM to use format callback functions.
>
> This commit introduces a new CopyFromRoutine struct, which is a set of
> callback routines to read tuples in a specific format. It also makes
> COPY FROM with the existing formats (text, CSV, and binary) utilize
> these format callbacks.
>
> This change is a preliminary step towards making the COPY FROM command
> extensible in terms of input formats.
>
> Similar to 2e4127b6d2d, this refactoring contributes to a performance
> improvement by reducing the number of "if" branches that need to be
> checked on a per-row basis when sending field representations in text
> or CSV mode. The performance benchmark results showed ~5% performance
> gain in text or CSV mode.
>
> Author: Sutou Kouhei <kou@clear-code.com>
> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
> Reviewed-by: Michael Paquier <michael@paquier.xyz>
> Reviewed-by: Andres Freund <andres@anarazel.de>
> Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
> Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
> Discussion: https://postgr.es/m/20231204.153548.2126325458835528809.kou@clear-code.com
>
>
>
> This patch has completely broken the file_textarray fdw, which uses NextCopyFromRawFields(). Removing that from API is not a good thing.
>
Thank you for pointing it out.
I've just posted my analysis[1] and am planning to revive that API
(Sutou-san already proposed an idea). Could you please check if the
idea would work for file_text_array_fdw?
Regards,
[1] https://www.postgresql.org/message-id/CAD21AoDr13%3Ddx%2Bk8gmQnR5_bY%2BNskyN4mbSWN0KhQncL6xuPMA%40ma...
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 20:14 Andrew Dunstan <andrew@dunslane.net>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Dunstan @ 2025-02-28 20:14 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Masahiko Sawada <msawada@postgresql.org>; pgsql-committers@lists.postgresql.org
On 2025-02-28 Fr 2:55 PM, Masahiko Sawada wrote:
> On Fri, Feb 28, 2025 at 11:47 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> On 2025-02-28 Fr 1:31 PM, Masahiko Sawada wrote:
>>
>> Refactor COPY FROM to use format callback functions.
>>
>> This commit introduces a new CopyFromRoutine struct, which is a set of
>> callback routines to read tuples in a specific format. It also makes
>> COPY FROM with the existing formats (text, CSV, and binary) utilize
>> these format callbacks.
>>
>> This change is a preliminary step towards making the COPY FROM command
>> extensible in terms of input formats.
>>
>> Similar to 2e4127b6d2d, this refactoring contributes to a performance
>> improvement by reducing the number of "if" branches that need to be
>> checked on a per-row basis when sending field representations in text
>> or CSV mode. The performance benchmark results showed ~5% performance
>> gain in text or CSV mode.
>>
>> Author: Sutou Kouhei <kou@clear-code.com>
>> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
>> Reviewed-by: Michael Paquier <michael@paquier.xyz>
>> Reviewed-by: Andres Freund <andres@anarazel.de>
>> Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
>> Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
>> Discussion: https://postgr.es/m/20231204.153548.2126325458835528809.kou@clear-code.com
>>
>>
>>
>> This patch has completely broken the file_textarray fdw, which uses NextCopyFromRawFields(). Removing that from API is not a good thing.
>>
> Thank you for pointing it out.
>
> I've just posted my analysis[1] and am planning to revive that API
> (Sutou-san already proposed an idea). Could you please check if the
> idea would work for file_text_array_fdw?
>
Looks OK, I think. You could even use the Internal function further down
in the file and avoid a function call.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 20:56 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2025-02-28 20:56 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Masahiko Sawada <msawada@postgresql.org>; pgsql-committers@lists.postgresql.org
On Fri, Feb 28, 2025 at 12:14 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-02-28 Fr 2:55 PM, Masahiko Sawada wrote:
> > On Fri, Feb 28, 2025 at 11:47 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> >>
> >> On 2025-02-28 Fr 1:31 PM, Masahiko Sawada wrote:
> >>
> >> Refactor COPY FROM to use format callback functions.
> >>
> >> This commit introduces a new CopyFromRoutine struct, which is a set of
> >> callback routines to read tuples in a specific format. It also makes
> >> COPY FROM with the existing formats (text, CSV, and binary) utilize
> >> these format callbacks.
> >>
> >> This change is a preliminary step towards making the COPY FROM command
> >> extensible in terms of input formats.
> >>
> >> Similar to 2e4127b6d2d, this refactoring contributes to a performance
> >> improvement by reducing the number of "if" branches that need to be
> >> checked on a per-row basis when sending field representations in text
> >> or CSV mode. The performance benchmark results showed ~5% performance
> >> gain in text or CSV mode.
> >>
> >> Author: Sutou Kouhei <kou@clear-code.com>
> >> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
> >> Reviewed-by: Michael Paquier <michael@paquier.xyz>
> >> Reviewed-by: Andres Freund <andres@anarazel.de>
> >> Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
> >> Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
> >> Discussion: https://postgr.es/m/20231204.153548.2126325458835528809.kou@clear-code.com
> >>
> >>
> >>
> >> This patch has completely broken the file_textarray fdw, which uses NextCopyFromRawFields(). Removing that from API is not a good thing.
> >>
> > Thank you for pointing it out.
> >
> > I've just posted my analysis[1] and am planning to revive that API
> > (Sutou-san already proposed an idea). Could you please check if the
> > idea would work for file_text_array_fdw?
> >
>
> Looks OK, I think. You could even use the Internal function further down
> in the file and avoid a function call.
Right. I've attached the updated patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
0001-Re-export-NextCopyFromRawFields-to-copy.h.patch (application/octet-stream, 3.8K) download
From 03ff27129c09358559cc6d9f1b0ce389daa9f6e1 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 28 Feb 2025 12:43:29 -0800
Subject: [PATCH] Re-export NextCopyFromRawFields() to copy.h.
Commit 7717f630069 removed NextCopyFromRawFields() from copy.h. While
it was hoped that NextCopyFrom() could serve as an alternative,
certain use cases still require NextCopyFromRawFields(). For instance,
extensions like file_text_array_fdw, which process source data with an
unknown number of columns, rely on this function.
Per buildfarm member crake.
Reported-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/5c7e1ac8-5083-4c08-af19-cb9ade2f16ce@dunslane.net
---
src/backend/commands/copyfromparse.c | 20 +++++++++++++++++---
src/include/commands/copy.h | 2 ++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index bad577aa67b..9f81557f245 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -152,6 +152,10 @@ static pg_attribute_always_inline bool CopyFromTextLikeOneRow(CopyFromState csta
Datum *values,
bool *nulls,
bool is_csv);
+static pg_attribute_always_inline bool NextCopyFromRawFieldsInternal(CopyFromState cstate,
+ char ***fields,
+ int *nfields,
+ bool is_csv);
/* Low-level communications functions */
@@ -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().
*
* An internal temporary buffer is returned via 'fields'. It is valid until
* the next call of the function. Since the function returns all raw fields
@@ -747,10 +760,11 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
* NOTE: force_not_null option are not applied to the returned fields.
*
* We use pg_attribute_always_inline to reduce function call overhead
- * and to help compilers to optimize away the 'is_csv' condition.
+ * and to help compilers to optimize away the 'is_csv' condition when called
+ * by internal functions such as CopyFromTextLikeOneRow().
*/
static pg_attribute_always_inline bool
-NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
+NextCopyFromRawFieldsInternal(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
{
int fldct;
bool done;
@@ -934,7 +948,7 @@ CopyFromTextLikeOneRow(CopyFromState cstate, ExprContext *econtext,
attr_count = list_length(cstate->attnumlist);
/* read raw fields in the next line */
- if (!NextCopyFromRawFields(cstate, &field_strings, &fldct, is_csv))
+ if (!NextCopyFromRawFieldsInternal(cstate, &field_strings, &fldct, is_csv))
return false;
/* check for overflowing fields */
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 7bc044e2816..f8784eb9554 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -107,6 +107,8 @@ extern CopyFromState BeginCopyFrom(ParseState *pstate, Relation rel, Node *where
extern void EndCopyFrom(CopyFromState cstate);
extern bool NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
Datum *values, bool *nulls);
+extern bool NextCopyFromRawFields(CopyFromState cstate,
+ char ***fields, int *nfields, bool is_csv);
extern void CopyFromErrorCallback(void *arg);
extern char *CopyLimitPrintoutLength(const char *str);
--
2.43.5
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 22:16 Sutou Kouhei <kou@clear-code.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Sutou Kouhei @ 2025-02-28 22:16 UTC (permalink / raw)
To: sawada.mshk@gmail.com; +Cc: andrew@dunslane.net; msawada@postgresql.org; pgsql-committers@lists.postgresql.org
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);
}
> @@ -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()
{
...
}
Thanks,
--
kou
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 22:39 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Sutou Kouhei <kou@clear-code.com>
0 siblings, 1 reply; 9+ messages in thread
From: Masahiko Sawada @ 2025-02-28 22:39 UTC (permalink / raw)
To: Sutou Kouhei <kou@clear-code.com>; +Cc: andrew@dunslane.net; msawada@postgresql.org; pgsql-committers@lists.postgresql.org
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.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Re-export-NextCopyFromRawFields-to-copy.h.patch (application/octet-stream, 4.1K) download
From 3e83a3915e60690dfae1dd30dab7f828f6102054 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 28 Feb 2025 12:43:29 -0800
Subject: [PATCH v2] Re-export NextCopyFromRawFields() to copy.h.
Commit 7717f630069 removed NextCopyFromRawFields() from copy.h. While
it was hoped that NextCopyFrom() could serve as an alternative,
certain use cases still require NextCopyFromRawFields(). For instance,
extensions like file_text_array_fdw, which process source data with an
unknown number of columns, rely on this function.
Per buildfarm member crake.
Reported-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: Sutou Kouhei <kou@clear-code.com>
Discussion: https://postgr.es/m/5c7e1ac8-5083-4c08-af19-cb9ade2f16ce@dunslane.net
---
src/backend/commands/copyfromparse.c | 28 +++++++++++++++++++++++-----
src/include/commands/copy.h | 2 ++
2 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index bad577aa67b..e8128f85e6b 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -152,6 +152,10 @@ static pg_attribute_always_inline bool CopyFromTextLikeOneRow(CopyFromState csta
Datum *values,
bool *nulls,
bool is_csv);
+static pg_attribute_always_inline bool NextCopyFromRawFieldsInternal(CopyFromState cstate,
+ char ***fields,
+ int *nfields,
+ bool is_csv);
/* Low-level communications functions */
@@ -736,8 +740,21 @@ 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.
+ * This function is exposed for use by extensions that read raw fields in the
+ * next line. See NextCopyFromRawFieldsInternal() for details.
+ */
+bool
+NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
+{
+ return NextCopyFromRawFieldsInternal(cstate, fields, nfields,
+ cstate->opts.csv_mode);
+}
+
+/*
+ * Workhorse for NextCopyFromRawFields().
+ *
+ * Read raw fields in the next line for COPY FROM in text or csv mode. Return
+ * false if no more lines.
*
* An internal temporary buffer is returned via 'fields'. It is valid until
* the next call of the function. Since the function returns all raw fields
@@ -747,10 +764,11 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
* NOTE: force_not_null option are not applied to the returned fields.
*
* We use pg_attribute_always_inline to reduce function call overhead
- * and to help compilers to optimize away the 'is_csv' condition.
+ * and to help compilers to optimize away the 'is_csv' condition when called
+ * by internal functions such as CopyFromTextLikeOneRow().
*/
static pg_attribute_always_inline bool
-NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
+NextCopyFromRawFieldsInternal(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
{
int fldct;
bool done;
@@ -934,7 +952,7 @@ CopyFromTextLikeOneRow(CopyFromState cstate, ExprContext *econtext,
attr_count = list_length(cstate->attnumlist);
/* read raw fields in the next line */
- if (!NextCopyFromRawFields(cstate, &field_strings, &fldct, is_csv))
+ if (!NextCopyFromRawFieldsInternal(cstate, &field_strings, &fldct, is_csv))
return false;
/* check for overflowing fields */
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 7bc044e2816..06dfdfef721 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -107,6 +107,8 @@ extern CopyFromState BeginCopyFrom(ParseState *pstate, Relation rel, Node *where
extern void EndCopyFrom(CopyFromState cstate);
extern bool NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
Datum *values, bool *nulls);
+extern bool NextCopyFromRawFields(CopyFromState cstate,
+ char ***fields, int *nfields);
extern void CopyFromErrorCallback(void *arg);
extern char *CopyLimitPrintoutLength(const char *str);
--
2.43.5
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 23:06 Andrew Dunstan <andrew@dunslane.net>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Dunstan @ 2025-02-28 23:06 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; Sutou Kouhei <kou@clear-code.com>; +Cc: msawada@postgresql.org; pgsql-committers@lists.postgresql.org
On 2025-02-28 Fr 5:39 PM, Masahiko Sawada wrote:
> 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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: pgsql: Refactor COPY FROM to use format callback functions.
@ 2025-02-28 23:35 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Andrew Dunstan <andrew@dunslane.net>
0 siblings, 0 replies; 9+ messages in thread
From: Masahiko Sawada @ 2025-02-28 23:35 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: Sutou Kouhei <kou@clear-code.com>; msawada@postgresql.org; pgsql-committers@lists.postgresql.org
On Fri, Feb 28, 2025 at 3:06 PM Andrew Dunstan <andrew@dunslane.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@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.
>
Thank you for the confirmation. Pushed the fix.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2025-02-28 23:35 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-02-28 18:31 pgsql: Refactor COPY FROM to use format callback functions. Masahiko Sawada <msawada@postgresql.org>
2025-02-28 19:46 ` Andrew Dunstan <andrew@dunslane.net>
2025-02-28 19:55 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-02-28 20:14 ` Andrew Dunstan <andrew@dunslane.net>
2025-02-28 20:56 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-02-28 22:16 ` Sutou Kouhei <kou@clear-code.com>
2025-02-28 22:39 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-02-28 23:06 ` Andrew Dunstan <andrew@dunslane.net>
2025-02-28 23:35 ` Masahiko Sawada <sawada.mshk@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox