public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 4/7] cirrus: code coverage
19+ messages / 6 participants
[nested] [flat]
* [PATCH 4/7] cirrus: code coverage
@ 2022-01-17 06:54 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Justin Pryzby @ 2022-01-17 06:54 UTC (permalink / raw)
ci-os-only: linux
---
.cirrus.yml | 20 ++++++++++++++++++--
src/tools/ci/code-coverage-report | 25 +++++++++++++++++++++++++
2 files changed, 43 insertions(+), 2 deletions(-)
create mode 100755 src/tools/ci/code-coverage-report
diff --git a/.cirrus.yml b/.cirrus.yml
index a3af4a0a808..a5c3b97925f 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -26,6 +26,13 @@ env:
TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
PG_TEST_EXTRA: kerberos ldap ssl
+ # The commit that this branch is rebased on. There's no easy way to find this.
+ # This does the right thing for cfbot, which always squishes all patches into a single commit.
+ # And does the right thing for any 1-patch commits.
+ # Patches series manually submitted to cirrus may benefit from setting this to the
+ # number of patches in the series (or directly to the commit the series was rebased on).
+ BASE_COMMIT: HEAD~1
+
# What files to preserve in case tests fail
on_failure: &on_failure
@@ -175,6 +182,7 @@ task:
cat /proc/cmdline
ulimit -a -H && ulimit -a -S
export
+ git diff --name-only "$BASE_COMMIT"
create_user_script: |
useradd -m postgres
chown -R postgres:postgres .
@@ -187,13 +195,14 @@ task:
chown root:postgres /tmp/cores
sysctl kernel.core_pattern='/tmp/cores/%e-%s-%p.core'
setup_additional_packages_script: |
- #apt-get update
- #apt-get -y install ...
+ apt-get update
+ apt-get -y install lcov
configure_script: |
su postgres <<-EOF
./configure \
--enable-cassert --enable-debug --enable-tap-tests \
+ --enable-coverage \
--enable-nls \
\
${LINUX_CONFIGURE_FEATURES} \
@@ -213,6 +222,13 @@ task:
make -s ${CHECK} ${CHECKFLAGS} -j${TEST_JOBS}
EOF
+ # Build coverage report for files changed since the base commit.
+ generate_coverage_report_script: |
+ src/tools/ci/code-coverage-report "$BASE_COMMIT"
+
+ coverage_artifacts:
+ paths: ['coverage/**/*.html', 'coverage/**/*.png', 'coverage/**/*.gcov', 'coverage/**/*.css' ]
+
on_failure:
<<: *on_failure
cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..57126247ea0
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,25 @@
+#! /bin/sh
+# Called during the linux CI task to generate a code coverage report.
+set -e
+
+base_branch=$1
+changed=`git diff --name-only "$base_branch" '*.c'`
+
+outdir=coverage
+mkdir "$outdir"
+
+# Coverage is shown only for changed files
+# This is useful to see coverage of newly-added code, but won't
+# show added/lost coverage in files which this patch doesn't modify.
+
+gcov=$outdir/coverage.gcov
+for f in $changed
+do
+ lcov --quiet --capture --directory "$f"
+done >"$gcov"
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov" ] || exit 0
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" --title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
--
2.17.1
--wLAMOaPNJ0fu1fTG
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0005-cirrus-build-docs-as-a-separate-task.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-01 05:31 Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-03-01 05:31 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Thu, Feb 22, 2024 at 06:39:48PM +0900, Sutou Kouhei wrote:
> If so, adding the change independently on HEAD makes
> sense. But I don't know why that improves
> performance... Inlining?
I guess so. It does not make much of a difference, though. The thing
is that the dispatch caused by the custom callbacks called for each
row is noticeable in any profiles I'm taking (not that much in the
worst-case scenarios, still a few percents), meaning that this impacts
the performance for all the in-core formats (text, csv, binary) as
long as we refactor text/csv/binary to use the routines of copyapi.h.
I don't really see a way forward, except if we don't dispatch the
in-core formats to not impact the default cases. That makes the code
a bit less elegant, but equally efficient for the existing formats.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-01 06:44 Sutou Kouhei <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-03-01 06:44 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 1 Mar 2024 14:31:38 +0900,
Michael Paquier <[email protected]> wrote:
> I guess so. It does not make much of a difference, though. The thing
> is that the dispatch caused by the custom callbacks called for each
> row is noticeable in any profiles I'm taking (not that much in the
> worst-case scenarios, still a few percents), meaning that this impacts
> the performance for all the in-core formats (text, csv, binary) as
> long as we refactor text/csv/binary to use the routines of copyapi.h.
> I don't really see a way forward, except if we don't dispatch the
> in-core formats to not impact the default cases. That makes the code
> a bit less elegant, but equally efficient for the existing formats.
It's an option based on your profile result but your
execution result also shows that v15 is faster than HEAD [1]:
> I am getting faster runtimes with v15 (6232ms in average)
> vs HEAD (6550ms) at 5M rows with COPY TO
[1] https://www.postgresql.org/message-id/flat/ZdbtQJ-p5H1_EDwE%40paquier.xyz#6439e6ad574f2d47cd7220e9bf...
I think that faster runtime is beneficial than mysterious
profile for users. So I think that we can merge v15 to
master.
Thanks,
--
kou
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-04 05:11 Sutou Kouhei <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-03-04 05:11 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 01 Mar 2024 15:44:43 +0900 (JST),
Sutou Kouhei <[email protected]> wrote:
>> I guess so. It does not make much of a difference, though. The thing
>> is that the dispatch caused by the custom callbacks called for each
>> row is noticeable in any profiles I'm taking (not that much in the
>> worst-case scenarios, still a few percents), meaning that this impacts
>> the performance for all the in-core formats (text, csv, binary) as
>> long as we refactor text/csv/binary to use the routines of copyapi.h.
>> I don't really see a way forward, except if we don't dispatch the
>> in-core formats to not impact the default cases. That makes the code
>> a bit less elegant, but equally efficient for the existing formats.
>
> It's an option based on your profile result but your
> execution result also shows that v15 is faster than HEAD [1]:
>
>> I am getting faster runtimes with v15 (6232ms in average)
>> vs HEAD (6550ms) at 5M rows with COPY TO
>
> [1] https://www.postgresql.org/message-id/flat/ZdbtQJ-p5H1_EDwE%40paquier.xyz#6439e6ad574f2d47cd7220e9bf...
>
> I think that faster runtime is beneficial than mysterious
> profile for users. So I think that we can merge v15 to
> master.
If this is a blocker of making COPY format extendable, can
we defer moving the existing text/csv/binary format
implementations to Copy{From,To}Routine for now as Michael
suggested to proceed making COPY format extendable? (Can we
add Copy{From,To}Routine without changing the existing
text/csv/binary format implementations?)
I attach a patch for it.
There is a large hunk for CopyOneRowTo() that is caused by
indent change. I also attach "...-w.patch" that uses "git
-w" to remove space only changes. "...-w.patch" is only for
review. We should use .patch without -w for push.
Thanks,
--
kou
Attachments:
[text/x-patch] v16-0001-Add-CopyFromRoutine-CopyToRountine.patch (12.6K, ../../[email protected]/2-v16-0001-Add-CopyFromRoutine-CopyToRountine.patch)
download | inline diff:
From 6a5bfc8e104f0a339b421028e9fec69a4d092671 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email protected]>
Date: Mon, 4 Mar 2024 13:52:34 +0900
Subject: [PATCH v16] Add CopyFromRoutine/CopyToRountine
They are for implementing custom COPY FROM/TO format. But this is not
enough to implement custom COPY FROM/TO format yet. We'll export some
APIs to receive/send data and add "format" option to COPY FROM/TO
later.
Existing text/csv/binary format implementations don't use
CopyFromRoutine/CopyToRoutine for now. We have a patch for it but we
defer it. Because there are some mysterious profile results in spite
of we get faster runtimes. See [1] for details.
[1] https://www.postgresql.org/message-id/ZdbtQJ-p5H1_EDwE%40paquier.xyz
Note that this doesn't change existing text/csv/binary format
implementations. There are many diffs for CopyOneRowTo() but they're
caused by indentation. They don't change implementations.
---
src/backend/commands/copyfrom.c | 24 +++++-
src/backend/commands/copyfromparse.c | 5 ++
src/backend/commands/copyto.c | 103 ++++++++++++++---------
src/include/commands/copyapi.h | 100 ++++++++++++++++++++++
src/include/commands/copyfrom_internal.h | 4 +
src/tools/pgindent/typedefs.list | 2 +
6 files changed, 193 insertions(+), 45 deletions(-)
create mode 100644 src/include/commands/copyapi.h
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index c3bc897028..9bf2f6497e 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1623,12 +1623,22 @@ BeginCopyFrom(ParseState *pstate,
/* Fetch the input function and typioparam info */
if (cstate->opts.binary)
+ {
getTypeBinaryInputInfo(att->atttypid,
&in_func_oid, &typioparams[attnum - 1]);
+ fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ }
+ else if (cstate->routine)
+ cstate->routine->CopyFromInFunc(cstate, att->atttypid,
+ &in_functions[attnum - 1],
+ &typioparams[attnum - 1]);
+
else
+ {
getTypeInputInfo(att->atttypid,
&in_func_oid, &typioparams[attnum - 1]);
- fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ }
/* Get default info if available */
defexprs[attnum - 1] = NULL;
@@ -1768,10 +1778,13 @@ BeginCopyFrom(ParseState *pstate,
/* Read and verify binary header */
ReceiveCopyBinaryHeader(cstate);
}
-
- /* create workspace for CopyReadAttributes results */
- if (!cstate->opts.binary)
+ else if (cstate->routine)
{
+ cstate->routine->CopyFromStart(cstate, tupDesc);
+ }
+ else
+ {
+ /* create workspace for CopyReadAttributes results */
AttrNumber attr_count = list_length(cstate->attnumlist);
cstate->max_fields = attr_count;
@@ -1789,6 +1802,9 @@ BeginCopyFrom(ParseState *pstate,
void
EndCopyFrom(CopyFromState cstate)
{
+ if (cstate->routine)
+ cstate->routine->CopyFromEnd(cstate);
+
/* No COPY FROM related resources except memory. */
if (cstate->is_program)
{
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 7cacd0b752..8b15080585 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -978,6 +978,11 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
Assert(fieldno == attr_count);
}
+ else if (cstate->routine)
+ {
+ if (!cstate->routine->CopyFromOneRow(cstate, econtext, values, nulls))
+ return false;
+ }
else
{
/* binary */
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 20ffc90363..6080627c83 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -24,6 +24,7 @@
#include "access/xact.h"
#include "access/xlog.h"
#include "commands/copy.h"
+#include "commands/copyapi.h"
#include "commands/progress.h"
#include "executor/execdesc.h"
#include "executor/executor.h"
@@ -71,6 +72,9 @@ typedef enum CopyDest
*/
typedef struct CopyToStateData
{
+ /* format routine */
+ const CopyToRoutine *routine;
+
/* low-level state data */
CopyDest copy_dest; /* type of copy source/destination */
FILE *copy_file; /* used if copy_dest == COPY_FILE */
@@ -777,14 +781,22 @@ DoCopyTo(CopyToState cstate)
Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
if (cstate->opts.binary)
+ {
getTypeBinaryOutputInfo(attr->atttypid,
&out_func_oid,
&isvarlena);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
+ else if (cstate->routine)
+ cstate->routine->CopyToOutFunc(cstate, attr->atttypid,
+ &cstate->out_functions[attnum - 1]);
else
+ {
getTypeOutputInfo(attr->atttypid,
&out_func_oid,
&isvarlena);
- fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
}
/*
@@ -811,6 +823,8 @@ DoCopyTo(CopyToState cstate)
tmp = 0;
CopySendInt32(cstate, tmp);
}
+ else if (cstate->routine)
+ cstate->routine->CopyToStart(cstate, tupDesc);
else
{
/*
@@ -892,6 +906,8 @@ DoCopyTo(CopyToState cstate)
/* Need to flush out the trailer */
CopySendEndOfRow(cstate);
}
+ else if (cstate->routine)
+ cstate->routine->CopyToEnd(cstate);
MemoryContextDelete(cstate->rowcontext);
@@ -916,61 +932,66 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
MemoryContextReset(cstate->rowcontext);
oldcontext = MemoryContextSwitchTo(cstate->rowcontext);
- if (cstate->opts.binary)
+ if (cstate->routine)
+ cstate->routine->CopyToOneRow(cstate, slot);
+ else
{
- /* Binary per-tuple header */
- CopySendInt16(cstate, list_length(cstate->attnumlist));
- }
-
- /* Make sure the tuple is fully deconstructed */
- slot_getallattrs(slot);
-
- foreach(cur, cstate->attnumlist)
- {
- int attnum = lfirst_int(cur);
- Datum value = slot->tts_values[attnum - 1];
- bool isnull = slot->tts_isnull[attnum - 1];
-
- if (!cstate->opts.binary)
+ if (cstate->opts.binary)
{
- if (need_delim)
- CopySendChar(cstate, cstate->opts.delim[0]);
- need_delim = true;
+ /* Binary per-tuple header */
+ CopySendInt16(cstate, list_length(cstate->attnumlist));
}
- if (isnull)
- {
- if (!cstate->opts.binary)
- CopySendString(cstate, cstate->opts.null_print_client);
- else
- CopySendInt32(cstate, -1);
- }
- else
+ /* Make sure the tuple is fully deconstructed */
+ slot_getallattrs(slot);
+
+ foreach(cur, cstate->attnumlist)
{
+ int attnum = lfirst_int(cur);
+ Datum value = slot->tts_values[attnum - 1];
+ bool isnull = slot->tts_isnull[attnum - 1];
+
if (!cstate->opts.binary)
{
- string = OutputFunctionCall(&out_functions[attnum - 1],
- value);
- if (cstate->opts.csv_mode)
- CopyAttributeOutCSV(cstate, string,
- cstate->opts.force_quote_flags[attnum - 1]);
+ if (need_delim)
+ CopySendChar(cstate, cstate->opts.delim[0]);
+ need_delim = true;
+ }
+
+ if (isnull)
+ {
+ if (!cstate->opts.binary)
+ CopySendString(cstate, cstate->opts.null_print_client);
else
- CopyAttributeOutText(cstate, string);
+ CopySendInt32(cstate, -1);
}
else
{
- bytea *outputbytes;
+ if (!cstate->opts.binary)
+ {
+ string = OutputFunctionCall(&out_functions[attnum - 1],
+ value);
+ if (cstate->opts.csv_mode)
+ CopyAttributeOutCSV(cstate, string,
+ cstate->opts.force_quote_flags[attnum - 1]);
+ else
+ CopyAttributeOutText(cstate, string);
+ }
+ else
+ {
+ bytea *outputbytes;
- outputbytes = SendFunctionCall(&out_functions[attnum - 1],
- value);
- CopySendInt32(cstate, VARSIZE(outputbytes) - VARHDRSZ);
- CopySendData(cstate, VARDATA(outputbytes),
- VARSIZE(outputbytes) - VARHDRSZ);
+ outputbytes = SendFunctionCall(&out_functions[attnum - 1],
+ value);
+ CopySendInt32(cstate, VARSIZE(outputbytes) - VARHDRSZ);
+ CopySendData(cstate, VARDATA(outputbytes),
+ VARSIZE(outputbytes) - VARHDRSZ);
+ }
}
}
- }
- CopySendEndOfRow(cstate);
+ CopySendEndOfRow(cstate);
+ }
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
new file mode 100644
index 0000000000..635c4cbff2
--- /dev/null
+++ b/src/include/commands/copyapi.h
@@ -0,0 +1,100 @@
+/*-------------------------------------------------------------------------
+ *
+ * copyapi.h
+ * API for COPY TO/FROM handlers
+ *
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/commands/copyapi.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef COPYAPI_H
+#define COPYAPI_H
+
+#include "executor/tuptable.h"
+#include "nodes/execnodes.h"
+
+/* These are private in commands/copy[from|to].c */
+typedef struct CopyFromStateData *CopyFromState;
+typedef struct CopyToStateData *CopyToState;
+
+/*
+ * API structure for a COPY FROM format implementation. Note this must be
+ * allocated in a server-lifetime manner, typically as a static const struct.
+ */
+typedef struct CopyFromRoutine
+{
+ /*
+ * Called when COPY FROM is started to set up the input functions
+ * associated to the relation's attributes writing to. `finfo` can be
+ * optionally filled to provide the catalog information of the input
+ * function. `typioparam` can be optionally filled to define the OID of
+ * the type to pass to the input function. `atttypid` is the OID of data
+ * type used by the relation's attribute.
+ */
+ void (*CopyFromInFunc) (CopyFromState cstate, Oid atttypid,
+ FmgrInfo *finfo, Oid *typioparam);
+
+ /*
+ * Called when COPY FROM is started.
+ *
+ * `tupDesc` is the tuple descriptor of the relation where the data needs
+ * to be copied. This can be used for any initialization steps required
+ * by a format.
+ */
+ void (*CopyFromStart) (CopyFromState cstate, TupleDesc tupDesc);
+
+ /*
+ * Copy one row to a set of `values` and `nulls` of size tupDesc->natts.
+ *
+ * 'econtext' is used to evaluate default expression for each column that
+ * is either not read from the file or is using the DEFAULT option of COPY
+ * FROM. It is NULL if no default values are used.
+ *
+ * Returns false if there are no more tuples to copy.
+ */
+ bool (*CopyFromOneRow) (CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls);
+
+ /* Called when COPY FROM has ended. */
+ void (*CopyFromEnd) (CopyFromState cstate);
+} CopyFromRoutine;
+
+/*
+ * API structure for a COPY TO format implementation. Note this must be
+ * allocated in a server-lifetime manner, typically as a static const struct.
+ */
+typedef struct CopyToRoutine
+{
+ /*
+ * Called when COPY TO is started to set up the output functions
+ * associated to the relation's attributes reading from. `finfo` can be
+ * optionally filled. `atttypid` is the OID of data type used by the
+ * relation's attribute.
+ */
+ void (*CopyToOutFunc) (CopyToState cstate, Oid atttypid,
+ FmgrInfo *finfo);
+
+ /*
+ * Called when COPY TO is started.
+ *
+ * `tupDesc` is the tuple descriptor of the relation from where the data
+ * is read.
+ */
+ void (*CopyToStart) (CopyToState cstate, TupleDesc tupDesc);
+
+ /*
+ * Copy one row for COPY TO.
+ *
+ * `slot` is the tuple slot where the data is emitted.
+ */
+ void (*CopyToOneRow) (CopyToState cstate, TupleTableSlot *slot);
+
+ /* Called when COPY TO has ended */
+ void (*CopyToEnd) (CopyToState cstate);
+} CopyToRoutine;
+
+#endif /* COPYAPI_H */
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index cad52fcc78..509b9e92a1 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -15,6 +15,7 @@
#define COPYFROM_INTERNAL_H
#include "commands/copy.h"
+#include "commands/copyapi.h"
#include "commands/trigger.h"
#include "nodes/miscnodes.h"
@@ -58,6 +59,9 @@ typedef enum CopyInsertMethod
*/
typedef struct CopyFromStateData
{
+ /* format routine */
+ const CopyFromRoutine *routine;
+
/* low-level state data */
CopySource copy_src; /* type of copy source */
FILE *copy_file; /* used if copy_src == COPY_FILE */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ee40a341d3..a5ae161ca5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -475,6 +475,7 @@ ConvertRowtypeExpr
CookedConstraint
CopyDest
CopyFormatOptions
+CopyFromRoutine
CopyFromState
CopyFromStateData
CopyHeaderChoice
@@ -484,6 +485,7 @@ CopyMultiInsertInfo
CopyOnErrorChoice
CopySource
CopyStmt
+CopyToRoutine
CopyToState
CopyToStateData
Cost
--
2.43.0
[text/x-patch] v16-0001-Add-CopyFromRoutine-CopyToRountine-w.patch (10.2K, ../../[email protected]/3-v16-0001-Add-CopyFromRoutine-CopyToRountine-w.patch)
download | inline diff:
From 6a5bfc8e104f0a339b421028e9fec69a4d092671 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email protected]>
Date: Mon, 4 Mar 2024 13:52:34 +0900
Subject: [PATCH v16] Add CopyFromRoutine/CopyToRountine
They are for implementing custom COPY FROM/TO format. But this is not
enough to implement custom COPY FROM/TO format yet. We'll export some
APIs to receive/send data and add "format" option to COPY FROM/TO
later.
Existing text/csv/binary format implementations don't use
CopyFromRoutine/CopyToRoutine for now. We have a patch for it but we
defer it. Because there are some mysterious profile results in spite
of we get faster runtimes. See [1] for details.
[1] https://www.postgresql.org/message-id/ZdbtQJ-p5H1_EDwE%40paquier.xyz
Note that this doesn't change existing text/csv/binary format
implementations. There are many diffs for CopyOneRowTo() but they're
caused by indentation. They don't change implementations.
---
src/backend/commands/copyfrom.c | 22 ++++-
src/backend/commands/copyfromparse.c | 5 ++
src/backend/commands/copyto.c | 21 +++++
src/include/commands/copyapi.h | 100 +++++++++++++++++++++++
src/include/commands/copyfrom_internal.h | 4 +
src/tools/pgindent/typedefs.list | 2 +
6 files changed, 151 insertions(+), 3 deletions(-)
create mode 100644 src/include/commands/copyapi.h
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index c3bc897028..9bf2f6497e 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1623,12 +1623,22 @@ BeginCopyFrom(ParseState *pstate,
/* Fetch the input function and typioparam info */
if (cstate->opts.binary)
+ {
getTypeBinaryInputInfo(att->atttypid,
&in_func_oid, &typioparams[attnum - 1]);
+ fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ }
+ else if (cstate->routine)
+ cstate->routine->CopyFromInFunc(cstate, att->atttypid,
+ &in_functions[attnum - 1],
+ &typioparams[attnum - 1]);
+
else
+ {
getTypeInputInfo(att->atttypid,
&in_func_oid, &typioparams[attnum - 1]);
fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ }
/* Get default info if available */
defexprs[attnum - 1] = NULL;
@@ -1768,10 +1778,13 @@ BeginCopyFrom(ParseState *pstate,
/* Read and verify binary header */
ReceiveCopyBinaryHeader(cstate);
}
-
- /* create workspace for CopyReadAttributes results */
- if (!cstate->opts.binary)
+ else if (cstate->routine)
{
+ cstate->routine->CopyFromStart(cstate, tupDesc);
+ }
+ else
+ {
+ /* create workspace for CopyReadAttributes results */
AttrNumber attr_count = list_length(cstate->attnumlist);
cstate->max_fields = attr_count;
@@ -1789,6 +1802,9 @@ BeginCopyFrom(ParseState *pstate,
void
EndCopyFrom(CopyFromState cstate)
{
+ if (cstate->routine)
+ cstate->routine->CopyFromEnd(cstate);
+
/* No COPY FROM related resources except memory. */
if (cstate->is_program)
{
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 7cacd0b752..8b15080585 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -978,6 +978,11 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
Assert(fieldno == attr_count);
}
+ else if (cstate->routine)
+ {
+ if (!cstate->routine->CopyFromOneRow(cstate, econtext, values, nulls))
+ return false;
+ }
else
{
/* binary */
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 20ffc90363..6080627c83 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -24,6 +24,7 @@
#include "access/xact.h"
#include "access/xlog.h"
#include "commands/copy.h"
+#include "commands/copyapi.h"
#include "commands/progress.h"
#include "executor/execdesc.h"
#include "executor/executor.h"
@@ -71,6 +72,9 @@ typedef enum CopyDest
*/
typedef struct CopyToStateData
{
+ /* format routine */
+ const CopyToRoutine *routine;
+
/* low-level state data */
CopyDest copy_dest; /* type of copy source/destination */
FILE *copy_file; /* used if copy_dest == COPY_FILE */
@@ -777,15 +781,23 @@ DoCopyTo(CopyToState cstate)
Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
if (cstate->opts.binary)
+ {
getTypeBinaryOutputInfo(attr->atttypid,
&out_func_oid,
&isvarlena);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
+ else if (cstate->routine)
+ cstate->routine->CopyToOutFunc(cstate, attr->atttypid,
+ &cstate->out_functions[attnum - 1]);
else
+ {
getTypeOutputInfo(attr->atttypid,
&out_func_oid,
&isvarlena);
fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
}
+ }
/*
* Create a temporary memory context that we can reset once per row to
@@ -811,6 +823,8 @@ DoCopyTo(CopyToState cstate)
tmp = 0;
CopySendInt32(cstate, tmp);
}
+ else if (cstate->routine)
+ cstate->routine->CopyToStart(cstate, tupDesc);
else
{
/*
@@ -892,6 +906,8 @@ DoCopyTo(CopyToState cstate)
/* Need to flush out the trailer */
CopySendEndOfRow(cstate);
}
+ else if (cstate->routine)
+ cstate->routine->CopyToEnd(cstate);
MemoryContextDelete(cstate->rowcontext);
@@ -916,6 +932,10 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
MemoryContextReset(cstate->rowcontext);
oldcontext = MemoryContextSwitchTo(cstate->rowcontext);
+ if (cstate->routine)
+ cstate->routine->CopyToOneRow(cstate, slot);
+ else
+ {
if (cstate->opts.binary)
{
/* Binary per-tuple header */
@@ -971,6 +991,7 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
}
CopySendEndOfRow(cstate);
+ }
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
new file mode 100644
index 0000000000..635c4cbff2
--- /dev/null
+++ b/src/include/commands/copyapi.h
@@ -0,0 +1,100 @@
+/*-------------------------------------------------------------------------
+ *
+ * copyapi.h
+ * API for COPY TO/FROM handlers
+ *
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/commands/copyapi.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef COPYAPI_H
+#define COPYAPI_H
+
+#include "executor/tuptable.h"
+#include "nodes/execnodes.h"
+
+/* These are private in commands/copy[from|to].c */
+typedef struct CopyFromStateData *CopyFromState;
+typedef struct CopyToStateData *CopyToState;
+
+/*
+ * API structure for a COPY FROM format implementation. Note this must be
+ * allocated in a server-lifetime manner, typically as a static const struct.
+ */
+typedef struct CopyFromRoutine
+{
+ /*
+ * Called when COPY FROM is started to set up the input functions
+ * associated to the relation's attributes writing to. `finfo` can be
+ * optionally filled to provide the catalog information of the input
+ * function. `typioparam` can be optionally filled to define the OID of
+ * the type to pass to the input function. `atttypid` is the OID of data
+ * type used by the relation's attribute.
+ */
+ void (*CopyFromInFunc) (CopyFromState cstate, Oid atttypid,
+ FmgrInfo *finfo, Oid *typioparam);
+
+ /*
+ * Called when COPY FROM is started.
+ *
+ * `tupDesc` is the tuple descriptor of the relation where the data needs
+ * to be copied. This can be used for any initialization steps required
+ * by a format.
+ */
+ void (*CopyFromStart) (CopyFromState cstate, TupleDesc tupDesc);
+
+ /*
+ * Copy one row to a set of `values` and `nulls` of size tupDesc->natts.
+ *
+ * 'econtext' is used to evaluate default expression for each column that
+ * is either not read from the file or is using the DEFAULT option of COPY
+ * FROM. It is NULL if no default values are used.
+ *
+ * Returns false if there are no more tuples to copy.
+ */
+ bool (*CopyFromOneRow) (CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls);
+
+ /* Called when COPY FROM has ended. */
+ void (*CopyFromEnd) (CopyFromState cstate);
+} CopyFromRoutine;
+
+/*
+ * API structure for a COPY TO format implementation. Note this must be
+ * allocated in a server-lifetime manner, typically as a static const struct.
+ */
+typedef struct CopyToRoutine
+{
+ /*
+ * Called when COPY TO is started to set up the output functions
+ * associated to the relation's attributes reading from. `finfo` can be
+ * optionally filled. `atttypid` is the OID of data type used by the
+ * relation's attribute.
+ */
+ void (*CopyToOutFunc) (CopyToState cstate, Oid atttypid,
+ FmgrInfo *finfo);
+
+ /*
+ * Called when COPY TO is started.
+ *
+ * `tupDesc` is the tuple descriptor of the relation from where the data
+ * is read.
+ */
+ void (*CopyToStart) (CopyToState cstate, TupleDesc tupDesc);
+
+ /*
+ * Copy one row for COPY TO.
+ *
+ * `slot` is the tuple slot where the data is emitted.
+ */
+ void (*CopyToOneRow) (CopyToState cstate, TupleTableSlot *slot);
+
+ /* Called when COPY TO has ended */
+ void (*CopyToEnd) (CopyToState cstate);
+} CopyToRoutine;
+
+#endif /* COPYAPI_H */
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index cad52fcc78..509b9e92a1 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -15,6 +15,7 @@
#define COPYFROM_INTERNAL_H
#include "commands/copy.h"
+#include "commands/copyapi.h"
#include "commands/trigger.h"
#include "nodes/miscnodes.h"
@@ -58,6 +59,9 @@ typedef enum CopyInsertMethod
*/
typedef struct CopyFromStateData
{
+ /* format routine */
+ const CopyFromRoutine *routine;
+
/* low-level state data */
CopySource copy_src; /* type of copy source */
FILE *copy_file; /* used if copy_src == COPY_FILE */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ee40a341d3..a5ae161ca5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -475,6 +475,7 @@ ConvertRowtypeExpr
CookedConstraint
CopyDest
CopyFormatOptions
+CopyFromRoutine
CopyFromState
CopyFromStateData
CopyHeaderChoice
@@ -484,6 +485,7 @@ CopyMultiInsertInfo
CopyOnErrorChoice
CopySource
CopyStmt
+CopyToRoutine
CopyToState
CopyToStateData
Cost
--
2.43.0
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-05 06:16 Michael Paquier <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-03-05 06:16 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Mon, Mar 04, 2024 at 02:11:08PM +0900, Sutou Kouhei wrote:
> If this is a blocker of making COPY format extendable, can
> we defer moving the existing text/csv/binary format
> implementations to Copy{From,To}Routine for now as Michael
> suggested to proceed making COPY format extendable? (Can we
> add Copy{From,To}Routine without changing the existing
> text/csv/binary format implementations?)
Yeah, I assume that it would be the way to go so as we don't do any
dispatching in default cases. A different approach that could be done
is to hide some of the parts of binary and text/csv in inline static
functions that are equivalent to the routine callbacks. That's
similar to the previous versions of the patch set, but if we come back
to the argument that there is a risk of blocking optimizations of more
of the local areas of the per-row processing in NextCopyFrom() and
CopyOneRowTo(), what you have sounds like a good balance.
CopyOneRowTo() could do something like that to avoid the extra
indentation:
if (cstate->routine)
{
cstate->routine->CopyToOneRow(cstate, slot);
MemoryContextSwitchTo(oldcontext);
return;
}
NextCopyFrom() does not need to be concerned by that.
> I attach a patch for it.
> There is a large hunk for CopyOneRowTo() that is caused by
> indent change. I also attach "...-w.patch" that uses "git
> -w" to remove space only changes. "...-w.patch" is only for
> review. We should use .patch without -w for push.
I didn't know this trick. That's indeed nice.. I may use that for
other stuff to make patches more presentable to the eyes. And that's
available as well with `git diff`.
If we basically agree about this part, how would the rest work out
with this set of APIs and the possibility to plug in a custom value
for FORMAT to do a pg_proc lookup, including an example of how these
APIs can be used?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-05 08:18 Sutou Kouhei <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-03-05 08:18 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Tue, 5 Mar 2024 15:16:33 +0900,
Michael Paquier <[email protected]> wrote:
> CopyOneRowTo() could do something like that to avoid the extra
> indentation:
> if (cstate->routine)
> {
> cstate->routine->CopyToOneRow(cstate, slot);
> MemoryContextSwitchTo(oldcontext);
> return;
> }
OK. The v17 patch uses this style. Others are same as the
v16.
> I didn't know this trick. That's indeed nice.. I may use that for
> other stuff to make patches more presentable to the eyes. And that's
> available as well with `git diff`.
:-)
> If we basically agree about this part, how would the rest work out
> with this set of APIs and the possibility to plug in a custom value
> for FORMAT to do a pg_proc lookup, including an example of how these
> APIs can be used?
I'll send the following patches after this patch is
merged. They are based on the v6 patch[1]:
1. Add copy_handler
* This also adds a pg_proc lookup for custom FORMAT
* This also adds a test for copy_handler
2. Export CopyToStateData
* We need it to implement custom copy TO handler
3. Add needed APIs to implement custom copy TO handler
* Add CopyToStateData::opaque
* Export CopySendEndOfRow()
4. Export CopyFromStateData
* We need it to implement custom copy FROM handler
5. Add needed APIs to implement custom copy FROM handler
* Add CopyFromStateData::opaque
* Export CopyReadBinaryData()
[1] https://www.postgresql.org/message-id/flat/20240124.144936.67229716500876806.kou%40clear-code.com#f1...
Thanks,
--
kou
Attachments:
[text/x-patch] v17-0001-Add-CopyFromRoutine-CopyToRountine.patch (10.1K, ../../[email protected]/2-v17-0001-Add-CopyFromRoutine-CopyToRountine.patch)
download | inline diff:
From a78b8ee88575e2c2873afc3acf3c8c4e535becf0 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email protected]>
Date: Mon, 4 Mar 2024 13:52:34 +0900
Subject: [PATCH v17] Add CopyFromRoutine/CopyToRountine
They are for implementing custom COPY FROM/TO format. But this is not
enough to implement custom COPY FROM/TO format yet. We'll export some
APIs to receive/send data and add "format" option to COPY FROM/TO
later.
Existing text/csv/binary format implementations don't use
CopyFromRoutine/CopyToRoutine for now. We have a patch for it but we
defer it. Because there are some mysterious profile results in spite
of we get faster runtimes. See [1] for details.
[1] https://www.postgresql.org/message-id/ZdbtQJ-p5H1_EDwE%40paquier.xyz
Note that this doesn't change existing text/csv/binary format
implementations.
---
src/backend/commands/copyfrom.c | 24 +++++-
src/backend/commands/copyfromparse.c | 5 ++
src/backend/commands/copyto.c | 25 +++++-
src/include/commands/copyapi.h | 100 +++++++++++++++++++++++
src/include/commands/copyfrom_internal.h | 4 +
src/tools/pgindent/typedefs.list | 2 +
6 files changed, 155 insertions(+), 5 deletions(-)
create mode 100644 src/include/commands/copyapi.h
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index c3bc897028..9bf2f6497e 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -1623,12 +1623,22 @@ BeginCopyFrom(ParseState *pstate,
/* Fetch the input function and typioparam info */
if (cstate->opts.binary)
+ {
getTypeBinaryInputInfo(att->atttypid,
&in_func_oid, &typioparams[attnum - 1]);
+ fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ }
+ else if (cstate->routine)
+ cstate->routine->CopyFromInFunc(cstate, att->atttypid,
+ &in_functions[attnum - 1],
+ &typioparams[attnum - 1]);
+
else
+ {
getTypeInputInfo(att->atttypid,
&in_func_oid, &typioparams[attnum - 1]);
- fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ fmgr_info(in_func_oid, &in_functions[attnum - 1]);
+ }
/* Get default info if available */
defexprs[attnum - 1] = NULL;
@@ -1768,10 +1778,13 @@ BeginCopyFrom(ParseState *pstate,
/* Read and verify binary header */
ReceiveCopyBinaryHeader(cstate);
}
-
- /* create workspace for CopyReadAttributes results */
- if (!cstate->opts.binary)
+ else if (cstate->routine)
{
+ cstate->routine->CopyFromStart(cstate, tupDesc);
+ }
+ else
+ {
+ /* create workspace for CopyReadAttributes results */
AttrNumber attr_count = list_length(cstate->attnumlist);
cstate->max_fields = attr_count;
@@ -1789,6 +1802,9 @@ BeginCopyFrom(ParseState *pstate,
void
EndCopyFrom(CopyFromState cstate)
{
+ if (cstate->routine)
+ cstate->routine->CopyFromEnd(cstate);
+
/* No COPY FROM related resources except memory. */
if (cstate->is_program)
{
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 7cacd0b752..8b15080585 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -978,6 +978,11 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
Assert(fieldno == attr_count);
}
+ else if (cstate->routine)
+ {
+ if (!cstate->routine->CopyFromOneRow(cstate, econtext, values, nulls))
+ return false;
+ }
else
{
/* binary */
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 20ffc90363..b4a7c9c8b9 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -24,6 +24,7 @@
#include "access/xact.h"
#include "access/xlog.h"
#include "commands/copy.h"
+#include "commands/copyapi.h"
#include "commands/progress.h"
#include "executor/execdesc.h"
#include "executor/executor.h"
@@ -71,6 +72,9 @@ typedef enum CopyDest
*/
typedef struct CopyToStateData
{
+ /* format routine */
+ const CopyToRoutine *routine;
+
/* low-level state data */
CopyDest copy_dest; /* type of copy source/destination */
FILE *copy_file; /* used if copy_dest == COPY_FILE */
@@ -777,14 +781,22 @@ DoCopyTo(CopyToState cstate)
Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
if (cstate->opts.binary)
+ {
getTypeBinaryOutputInfo(attr->atttypid,
&out_func_oid,
&isvarlena);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
+ else if (cstate->routine)
+ cstate->routine->CopyToOutFunc(cstate, attr->atttypid,
+ &cstate->out_functions[attnum - 1]);
else
+ {
getTypeOutputInfo(attr->atttypid,
&out_func_oid,
&isvarlena);
- fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
}
/*
@@ -811,6 +823,8 @@ DoCopyTo(CopyToState cstate)
tmp = 0;
CopySendInt32(cstate, tmp);
}
+ else if (cstate->routine)
+ cstate->routine->CopyToStart(cstate, tupDesc);
else
{
/*
@@ -892,6 +906,8 @@ DoCopyTo(CopyToState cstate)
/* Need to flush out the trailer */
CopySendEndOfRow(cstate);
}
+ else if (cstate->routine)
+ cstate->routine->CopyToEnd(cstate);
MemoryContextDelete(cstate->rowcontext);
@@ -916,6 +932,13 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
MemoryContextReset(cstate->rowcontext);
oldcontext = MemoryContextSwitchTo(cstate->rowcontext);
+ if (cstate->routine)
+ {
+ cstate->routine->CopyToOneRow(cstate, slot);
+ MemoryContextSwitchTo(oldcontext);
+ return;
+ }
+
if (cstate->opts.binary)
{
/* Binary per-tuple header */
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
new file mode 100644
index 0000000000..635c4cbff2
--- /dev/null
+++ b/src/include/commands/copyapi.h
@@ -0,0 +1,100 @@
+/*-------------------------------------------------------------------------
+ *
+ * copyapi.h
+ * API for COPY TO/FROM handlers
+ *
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/commands/copyapi.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef COPYAPI_H
+#define COPYAPI_H
+
+#include "executor/tuptable.h"
+#include "nodes/execnodes.h"
+
+/* These are private in commands/copy[from|to].c */
+typedef struct CopyFromStateData *CopyFromState;
+typedef struct CopyToStateData *CopyToState;
+
+/*
+ * API structure for a COPY FROM format implementation. Note this must be
+ * allocated in a server-lifetime manner, typically as a static const struct.
+ */
+typedef struct CopyFromRoutine
+{
+ /*
+ * Called when COPY FROM is started to set up the input functions
+ * associated to the relation's attributes writing to. `finfo` can be
+ * optionally filled to provide the catalog information of the input
+ * function. `typioparam` can be optionally filled to define the OID of
+ * the type to pass to the input function. `atttypid` is the OID of data
+ * type used by the relation's attribute.
+ */
+ void (*CopyFromInFunc) (CopyFromState cstate, Oid atttypid,
+ FmgrInfo *finfo, Oid *typioparam);
+
+ /*
+ * Called when COPY FROM is started.
+ *
+ * `tupDesc` is the tuple descriptor of the relation where the data needs
+ * to be copied. This can be used for any initialization steps required
+ * by a format.
+ */
+ void (*CopyFromStart) (CopyFromState cstate, TupleDesc tupDesc);
+
+ /*
+ * Copy one row to a set of `values` and `nulls` of size tupDesc->natts.
+ *
+ * 'econtext' is used to evaluate default expression for each column that
+ * is either not read from the file or is using the DEFAULT option of COPY
+ * FROM. It is NULL if no default values are used.
+ *
+ * Returns false if there are no more tuples to copy.
+ */
+ bool (*CopyFromOneRow) (CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls);
+
+ /* Called when COPY FROM has ended. */
+ void (*CopyFromEnd) (CopyFromState cstate);
+} CopyFromRoutine;
+
+/*
+ * API structure for a COPY TO format implementation. Note this must be
+ * allocated in a server-lifetime manner, typically as a static const struct.
+ */
+typedef struct CopyToRoutine
+{
+ /*
+ * Called when COPY TO is started to set up the output functions
+ * associated to the relation's attributes reading from. `finfo` can be
+ * optionally filled. `atttypid` is the OID of data type used by the
+ * relation's attribute.
+ */
+ void (*CopyToOutFunc) (CopyToState cstate, Oid atttypid,
+ FmgrInfo *finfo);
+
+ /*
+ * Called when COPY TO is started.
+ *
+ * `tupDesc` is the tuple descriptor of the relation from where the data
+ * is read.
+ */
+ void (*CopyToStart) (CopyToState cstate, TupleDesc tupDesc);
+
+ /*
+ * Copy one row for COPY TO.
+ *
+ * `slot` is the tuple slot where the data is emitted.
+ */
+ void (*CopyToOneRow) (CopyToState cstate, TupleTableSlot *slot);
+
+ /* Called when COPY TO has ended */
+ void (*CopyToEnd) (CopyToState cstate);
+} CopyToRoutine;
+
+#endif /* COPYAPI_H */
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index cad52fcc78..509b9e92a1 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -15,6 +15,7 @@
#define COPYFROM_INTERNAL_H
#include "commands/copy.h"
+#include "commands/copyapi.h"
#include "commands/trigger.h"
#include "nodes/miscnodes.h"
@@ -58,6 +59,9 @@ typedef enum CopyInsertMethod
*/
typedef struct CopyFromStateData
{
+ /* format routine */
+ const CopyFromRoutine *routine;
+
/* low-level state data */
CopySource copy_src; /* type of copy source */
FILE *copy_file; /* used if copy_src == COPY_FILE */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ee40a341d3..a5ae161ca5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -475,6 +475,7 @@ ConvertRowtypeExpr
CookedConstraint
CopyDest
CopyFormatOptions
+CopyFromRoutine
CopyFromState
CopyFromStateData
CopyHeaderChoice
@@ -484,6 +485,7 @@ CopyMultiInsertInfo
CopyOnErrorChoice
CopySource
CopyStmt
+CopyToRoutine
CopyToState
CopyToStateData
Cost
--
2.43.0
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-06 06:34 Michael Paquier <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-03-06 06:34 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Tue, Mar 05, 2024 at 05:18:08PM +0900, Sutou Kouhei wrote:
> I'll send the following patches after this patch is
> merged.
I am not sure that my schedule is on track to allow that for this
release, unfortunately, especially with all the other items to review
and discuss to make this thread feature-complete. There should be
a bit more than four weeks until the feature freeze (date not set in
stone, should be around the 8th of April AoE), but I have less than
the half due to personal issues. Perhaps if somebody jumps on this
thread, that will be possible..
> They are based on the v6 patch[1]:
>
> 1. Add copy_handler
> * This also adds a pg_proc lookup for custom FORMAT
> * This also adds a test for copy_handler
> 2. Export CopyToStateData
> * We need it to implement custom copy TO handler
> 3. Add needed APIs to implement custom copy TO handler
> * Add CopyToStateData::opaque
> * Export CopySendEndOfRow()
> 4. Export CopyFromStateData
> * We need it to implement custom copy FROM handler
> 5. Add needed APIs to implement custom copy FROM handler
> * Add CopyFromStateData::opaque
> * Export CopyReadBinaryData()
Hmm. Sounds like a good plan for a split.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-07 06:32 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-03-07 06:32 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Wed, Mar 06, 2024 at 03:34:04PM +0900, Michael Paquier wrote:
> I am not sure that my schedule is on track to allow that for this
> release, unfortunately, especially with all the other items to review
> and discuss to make this thread feature-complete. There should be
> a bit more than four weeks until the feature freeze (date not set in
> stone, should be around the 8th of April AoE), but I have less than
> the half due to personal issues. Perhaps if somebody jumps on this
> thread, that will be possible..
While on it, here are some profiles based on HEAD and v17 with the
previous tests (COPY TO /dev/null, COPY FROM data sent to the void).
COPY FROM, text format with 30 attributes and HEAD:
- 66.53% 16.33% postgres postgres [.] NextCopyFrom
- 50.20% NextCopyFrom
- 30.83% NextCopyFromRawFields
+ 16.09% CopyReadLine
13.72% CopyReadAttributesText
+ 19.11% InputFunctionCallSafe
+ 16.33% _start
COPY FROM, text format with 30 attributes and v17:
- 66.60% 16.10% postgres postgres [.] NextCopyFrom
- 50.50% NextCopyFrom
- 30.44% NextCopyFromRawFields
+ 15.71% CopyReadLine
13.73% CopyReadAttributesText
+ 19.81% InputFunctionCallSafe
+ 16.10% _start
COPY TO, text format with 30 attributes and HEAD:
- 79.55% 15.54% postgres postgres [.] CopyOneRowTo
- 64.01% CopyOneRowTo
+ 30.01% OutputFunctionCall
+ 11.71% appendBinaryStringInfo
9.36% CopyAttributeOutText
+ 3.03% CopySendEndOfRow
1.65% int4out
1.01% 0xffff83e46be4
0.93% 0xffff83e46be8
0.93% memcpy@plt
0.87% pgstat_progress_update_param
0.78% enlargeStringInfo
0.67% 0xffff83e46bb4
0.66% 0xffff83e46bcc
0.57% MemoryContextReset
+ 15.54% _start
COPY TO, text format with 30 attributes and v17:
- 79.35% 16.08% postgres postgres [.] CopyOneRowTo
- 62.27% CopyOneRowTo
+ 28.92% OutputFunctionCall
+ 10.88% appendBinaryStringInfo
9.54% CopyAttributeOutText
+ 3.03% CopySendEndOfRow
1.60% int4out
0.97% pgstat_progress_update_param
0.95% 0xffff8c46cbe8
0.89% memcpy@plt
0.87% 0xffff8c46cbe4
0.79% enlargeStringInfo
0.64% 0xffff8c46cbcc
0.61% 0xffff8c46cbb4
0.58% MemoryContextReset
+ 16.08% _start
So, in short, and that's not really a surprise, there is no effect
once we use the dispatching with the routines only when a format would
want to plug-in with the APIs, but a custom format would still have a
penalty of a few percents for both if bottlenecked on CPU.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-08 00:22 Sutou Kouhei <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Sutou Kouhei @ 2024-03-08 00:22 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 7 Mar 2024 15:32:01 +0900,
Michael Paquier <[email protected]> wrote:
> While on it, here are some profiles based on HEAD and v17 with the
> previous tests (COPY TO /dev/null, COPY FROM data sent to the void).
>
...
>
> So, in short, and that's not really a surprise, there is no effect
> once we use the dispatching with the routines only when a format would
> want to plug-in with the APIs, but a custom format would still have a
> penalty of a few percents for both if bottlenecked on CPU.
Thanks for sharing these profiles!
I agree with you.
This shows that the v17 approach doesn't affect the current
text/csv/binary implementations. (The v17 approach just adds
2 new structs, Copy{From,To}Rountine, without changing the
current text/csv/binary implementations.)
Can we push the v17 patch and proceed following
implementations? Could someone (especially a PostgreSQL
committer) take a look at this for double-check?
Thanks,
--
kou
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-11 00:00 jian he <[email protected]>
parent: Sutou Kouhei <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: jian he @ 2024-03-11 00:00 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Fri, Mar 8, 2024 at 8:23 AM Sutou Kouhei <[email protected]> wrote:
>
>
> This shows that the v17 approach doesn't affect the current
> text/csv/binary implementations. (The v17 approach just adds
> 2 new structs, Copy{From,To}Rountine, without changing the
> current text/csv/binary implementations.)
>
> Can we push the v17 patch and proceed following
> implementations? Could someone (especially a PostgreSQL
> committer) take a look at this for double-check?
>
Hi, here are my cents:
Currently in v17, we have 3 extra functions within DoCopyTo
CopyToStart, one time, start, doing some preliminary work.
CopyToOneRow, doing the repetitive work, called many times, row by row.
CopyToEnd, one time doing the closing work.
seems to need a function pointer for processing the format and other options.
or maybe the reason is we need a one time function call before doing DoCopyTo,
like one time initialization.
We can placed the function pointer after:
`
cstate = BeginCopyTo(pstate, rel, query, relid,
stmt->filename, stmt->is_program,
NULL, stmt->attlist, stmt->options);
`
generally in v17, the code pattern looks like this.
if (cstate->opts.binary)
{
/* handle binary format */
}
else if (cstate->routine)
{
/* custom code, make the copy format extensible */
}
else
{
/* handle non-binary, (csv or text) format */
}
maybe we need another bool flag like `bool buildin_format`.
if the copy format is {csv|text|binary} then buildin_format is true else false.
so the code pattern would be:
if (cstate->opts.binary)
{
/* handle binary format */
}
else if (cstate->routine && !buildin_format)
{
/* custom code, make the copy format extensible */
}
else
{
/* handle non-binary, (csv or text) format */
}
otherwise the {CopyToRoutine| CopyFromRoutine} needs a function pointer
to distinguish native copy format and extensible supported format,
like I mentioned above?
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-11 00:56 Sutou Kouhei <[email protected]>
parent: jian he <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-03-11 00:56 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
In <CACJufxEgn3=j-UWg-f2-DbLO+uVSKGcofpkX5trx+=YX6icSFg@mail.gmail.com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Mar 2024 08:00:00 +0800,
jian he <[email protected]> wrote:
> Hi, here are my cents:
> Currently in v17, we have 3 extra functions within DoCopyTo
> CopyToStart, one time, start, doing some preliminary work.
> CopyToOneRow, doing the repetitive work, called many times, row by row.
> CopyToEnd, one time doing the closing work.
>
> seems to need a function pointer for processing the format and other options.
> or maybe the reason is we need a one time function call before doing DoCopyTo,
> like one time initialization.
I know that JSON format wants it but can we defer it? We can
add more options later. I want to proceed this improvement
step by step.
More use cases will help us which callbacks are needed. We
will be able to collect more use cases by providing basic
callbacks.
> generally in v17, the code pattern looks like this.
> if (cstate->opts.binary)
> {
> /* handle binary format */
> }
> else if (cstate->routine)
> {
> /* custom code, make the copy format extensible */
> }
> else
> {
> /* handle non-binary, (csv or text) format */
> }
> maybe we need another bool flag like `bool buildin_format`.
> if the copy format is {csv|text|binary} then buildin_format is true else false.
>
> so the code pattern would be:
> if (cstate->opts.binary)
> {
> /* handle binary format */
> }
> else if (cstate->routine && !buildin_format)
> {
> /* custom code, make the copy format extensible */
> }
> else
> {
> /* handle non-binary, (csv or text) format */
> }
>
> otherwise the {CopyToRoutine| CopyFromRoutine} needs a function pointer
> to distinguish native copy format and extensible supported format,
> like I mentioned above?
Hmm. I may miss something but I think that we don't need the
bool flag. Because we don't set cstate->routine for native
copy formats. So we can distinguish native copy format and
extensible supported format by checking only
cstate->routine.
Thanks,
--
kou
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-13 08:00 jian he <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: jian he @ 2024-03-13 08:00 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Mon, Mar 11, 2024 at 8:56 AM Sutou Kouhei <[email protected]> wrote:
>
> Hi,
>
> In <CACJufxEgn3=j-UWg-f2-DbLO+uVSKGcofpkX5trx+=YX6icSFg@mail.gmail.com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 11 Mar 2024 08:00:00 +0800,
> jian he <[email protected]> wrote:
>
> > Hi, here are my cents:
> > Currently in v17, we have 3 extra functions within DoCopyTo
> > CopyToStart, one time, start, doing some preliminary work.
> > CopyToOneRow, doing the repetitive work, called many times, row by row.
> > CopyToEnd, one time doing the closing work.
> >
> > seems to need a function pointer for processing the format and other options.
> > or maybe the reason is we need a one time function call before doing DoCopyTo,
> > like one time initialization.
>
> I know that JSON format wants it but can we defer it? We can
> add more options later. I want to proceed this improvement
> step by step.
>
> More use cases will help us which callbacks are needed. We
> will be able to collect more use cases by providing basic
> callbacks.
I guess one of the ultimate goals would be that COPY can export data
to a customized format.
Let's say the customized format is "csv1", but it is just analogous to
the csv format.
people should be able to create an extension, with serval C functions,
then they can do `copy (select 1 ) to stdout (format 'csv1');`
but the output will be exact same as `copy (select 1 ) to stdout
(format 'csv');`
In such a scenario, we require a function akin to ProcessCopyOptions
to handle situations
where CopyFormatOptions->csv_mode is true, while the format is "csv1".
but CopyToStart is already within the DoCopyTo function, so you do
need an extra function pointer?
I do agree with the incremental improvement method.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-03-20 14:27 Sutou Kouhei <[email protected]>
parent: Sutou Kouhei <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-03-20 14:27 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
Could someone review the v17 patch to proceed this?
The v17 patch:
https://www.postgresql.org/message-id/flat/20240305.171808.667980402249336456.kou%40clear-code.com#d...
Some profiles by Michael:
https://www.postgresql.org/message-id/flat/ZelfYatRdVZN3FbE%40paquier.xyz#eccfd1a0131af93c48026d691c...
Thanks,
--
kou
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 08 Mar 2024 09:22:54 +0900 (JST),
Sutou Kouhei <[email protected]> wrote:
> Hi,
>
> In <[email protected]>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 7 Mar 2024 15:32:01 +0900,
> Michael Paquier <[email protected]> wrote:
>
>> While on it, here are some profiles based on HEAD and v17 with the
>> previous tests (COPY TO /dev/null, COPY FROM data sent to the void).
>>
> ...
>>
>> So, in short, and that's not really a surprise, there is no effect
>> once we use the dispatching with the routines only when a format would
>> want to plug-in with the APIs, but a custom format would still have a
>> penalty of a few percents for both if bottlenecked on CPU.
>
> Thanks for sharing these profiles!
> I agree with you.
>
> This shows that the v17 approach doesn't affect the current
> text/csv/binary implementations. (The v17 approach just adds
> 2 new structs, Copy{From,To}Rountine, without changing the
> current text/csv/binary implementations.)
>
> Can we push the v17 patch and proceed following
> implementations? Could someone (especially a PostgreSQL
> committer) take a look at this for double-check?
>
>
> Thanks,
> --
> kou
>
>
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-04-10 08:16 Sutou Kouhei <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-04-10 08:16 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi Andres,
Could you take a look at this? I think that you don't want
to touch the current text/csv/binary implementations. The
v17 patch approach doesn't touch the current text/csv/binary
implementations. What do you think about this approach?
Thanks,
--
kou
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 20 Mar 2024 23:27:32 +0900 (JST),
Sutou Kouhei <[email protected]> wrote:
> Hi,
>
> Could someone review the v17 patch to proceed this?
>
> The v17 patch:
> https://www.postgresql.org/message-id/flat/20240305.171808.667980402249336456.kou%40clear-code.com#d...
>
> Some profiles by Michael:
> https://www.postgresql.org/message-id/flat/ZelfYatRdVZN3FbE%40paquier.xyz#eccfd1a0131af93c48026d691c...
>
> Thanks,
> --
> kou
>
> In <[email protected]>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 08 Mar 2024 09:22:54 +0900 (JST),
> Sutou Kouhei <[email protected]> wrote:
>
>> Hi,
>>
>> In <[email protected]>
>> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 7 Mar 2024 15:32:01 +0900,
>> Michael Paquier <[email protected]> wrote:
>>
>>> While on it, here are some profiles based on HEAD and v17 with the
>>> previous tests (COPY TO /dev/null, COPY FROM data sent to the void).
>>>
>> ...
>>>
>>> So, in short, and that's not really a surprise, there is no effect
>>> once we use the dispatching with the routines only when a format would
>>> want to plug-in with the APIs, but a custom format would still have a
>>> penalty of a few percents for both if bottlenecked on CPU.
>>
>> Thanks for sharing these profiles!
>> I agree with you.
>>
>> This shows that the v17 approach doesn't affect the current
>> text/csv/binary implementations. (The v17 approach just adds
>> 2 new structs, Copy{From,To}Rountine, without changing the
>> current text/csv/binary implementations.)
>>
>> Can we push the v17 patch and proceed following
>> implementations? Could someone (especially a PostgreSQL
>> committer) take a look at this for double-check?
>>
>>
>> Thanks,
>> --
>> kou
>>
>>
>
>
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-07-19 12:40 Tomas Vondra <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Tomas Vondra @ 2024-07-19 12:40 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hello Kouhei-san,
I think it'd be helpful if you could post a patch status, i.e. a message
re-explaininig what it aims to achieve, summary of the discussion so
far, and what you think are the open questions. Otherwise every reviewer
has to read the whole thread to learn this.
FWIW I realize there are other related patches, and maybe some of the
discussion is happening on those threads. But that's just another reason
to post the summary here - as a reviewer I'm not going to read random
other patches that "might" have relevant info.
-----
The way I understand it, the ultimate goal is to allow extensions to
define formats using CREATE XYZ. And I agree that would be a very
valuable feature. But the proposed patch does not do that, right? It
only does some basic things at the C level, there's no DDL etc.
Per the commit message, none of the existing formats (text/csv/binary)
is implemented as "copy routine". IMHO that's a bit strange, because
that's exactly what I'd expect this patch to do - to define all the
infrastructure (catalogs, ...) and switch the existing formats to it.
Yes, the patch will be larger, but it'll also simplify some of the code
(right now there's a bunch of branches to handle these "old" formats).
How would you even know the new code is correct, when there's nothing
using using the "copy routine" branch?
In fact, doesn't this mean that the benchmarks presented earlier are not
very useful? We still use the old code, except there are a couple "if"
branches that are never taken? I don't think this measures the new
approach would not be slower once everything gets to be copy routine.
Or what am I missing?
Also, how do we know this API is suitable for the alternative formats?
For example you mentioned Arrow, and I suppose people will want to add
support for other column-oriented formats. I assume that will require
stashing a batch of rows (or some other internal state) somewhere, but
does the proposed API plan for that?
My guess would be we'll need to add a "private_data" pointer to the
CopyFromStateData/CopyToStateData structs, but maybe I'm wrong.
Also, won't the alternative formats require custom parameters. For
example, for column-oriented-formats it might be useful to specify a
stripe size (rows per batch), etc. I'm not saying this patch needs to
implement that, but maybe the API should expect it?
-----
To sum this up, what I think needs to happen for this patch to move forward:
1) Switch the existing formats to the new API, to validate the API works
at least for them, allow testing and benchmarking the code.
2) Try implementing some of the more exotic formats (column-oriented) to
test the API works for those too.
3) Maybe try implementing a PoC version to do the DDL, so that it
actually is extensible.
It's not my intent to "move the goalposts" - I think it's fine if the
patches (2) and (3) are just PoC, to validate (1) goes in the right
direction. For example, it's fine if (2) just hard-codes the new format
next to the build-in ones - that's not something we'd commit, I think,
but for validation of (1) it's good enough.
Most of the DDL stuff can probably be "copied" from FDW handlers. It's
pretty similar, and the "routine" idea is what FDW does too. It probably
also shows a good way to "initialize" the routine, etc.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-07-22 07:11 Li, Yong <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Li, Yong @ 2024-07-22 07:11 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Nathan Bossart <[email protected]>; pgsql-hackers; Tomas Vondra <[email protected]>
Hi Kou,
I tried to follow the thread but had to skip quite some discussions in the middle part of the thread. From what I read, it appears to me that there were a lot of back-and-forth discussions on the specific implementation details (i.e. do not touch existing format implementation), performance concerns and how to split the patches to make it more manageable.
My understanding is that the provided v17 patch aims to achieve the followings:
- Retain existing format implementations as built-in formats, and do not go through the new interface for them.
- Make sure that there is no sign of performance degradation.
- Refactoring the existing code to make it easier and possible to make copy handlers extensible. However, some of the infrastructure work that are required to make copy handler extensible are intentionally delayed for future patches. Some of the work were proposed as patches in earlier messages, but they were not explicitly referenced in recent messages.
Overall, the current v17 patch applies cleanly to HEAD. “make check-world” also runs cleanly. If my understanding of the current status of the patch is correct, the patch looks good to me.
Regards,
Yong
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-07-22 07:45 Sutou Kouhei <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Sutou Kouhei @ 2024-07-22 07:45 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi Tomas,
Thanks for joining this thread!
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 19 Jul 2024 14:40:05 +0200,
Tomas Vondra <[email protected]> wrote:
> I think it'd be helpful if you could post a patch status, i.e. a message
> re-explaininig what it aims to achieve, summary of the discussion so
> far, and what you think are the open questions. Otherwise every reviewer
> has to read the whole thread to learn this.
It makes sense. It seems your questions covers all important
points in this thread. So my answers of your questions
summarize the latest information.
> FWIW I realize there are other related patches, and maybe some of the
> discussion is happening on those threads. But that's just another reason
> to post the summary here - as a reviewer I'm not going to read random
> other patches that "might" have relevant info.
It makes sense too. To clarify it, other threads are
unrelated. We can focus on only this thread for this propose.
> The way I understand it, the ultimate goal is to allow extensions to
> define formats using CREATE XYZ.
Right.
> But the proposed patch does not do that, right? It
> only does some basic things at the C level, there's no DDL etc.
Right. The latest patch set includes only the basic things
for the first implementation.
> Per the commit message, none of the existing formats (text/csv/binary)
> is implemented as "copy routine".
Right.
> IMHO that's a bit strange, because
> that's exactly what I'd expect this patch to do - to define all the
> infrastructure (catalogs, ...) and switch the existing formats to it.
We did it in the v1-v15 patch sets. But the v16/v17 patch
sets remove it because of a profiling result. (It's
described later.)
In general, we don't want to decrease the current
performance of the existing formats:
https://www.postgresql.org/message-id/flat/10025bac-158c-ffe7-fbec-32b42629121f%40dunslane.net#81cf8...
> We've spent quite a lot of blood sweat and tears over the years to make
> COPY fast, and we should not sacrifice any of that lightly.
The v15 patch set is faster than HEAD but there is a
mysterious profiling result:
https://www.postgresql.org/message-id/flat/ZdbtQJ-p5H1_EDwE%40paquier.xyz#6439e6ad574f2d47cd7220e9bf...
> The increase in CopyOneRowTo from 80% to 85% worries me
...
> I am getting faster
> runtimes with v15 (6232ms in average) vs HEAD (6550ms).
I think that it's not a blocker because the v15 patch set
approach is faster. But someone may think that it's a
blocker. So the v16 or later patch sets don't include codes
to use this extension mechanism for the existing formats. We
can work on it after we introduce the basic features if it's
valuable.
> How would you even know the new code is correct, when there's nothing
> using using the "copy routine" branch?
We can't test it only with the v16/v17 patch set
changes. But we can do it by adding more changes we did in
the v6 patch set.
https://www.postgresql.org/message-id/flat/20240124.144936.67229716500876806.kou%40clear-code.com#f1...
If we should commit the basic changes with tests, I can
adjust the test mechanism in v6 patch set and add it to the
latest patch set. But it needs CREATE XYZ mechanism and
so on too. Is it OK?
> In fact, doesn't this mean that the benchmarks presented earlier are not
> very useful? We still use the old code, except there are a couple "if"
> branches that are never taken? I don't think this measures the new
> approach would not be slower once everything gets to be copy routine.
Here is a benchmark result with the v17 and HEAD:
https://www.postgresql.org/message-id/flat/ZelfYatRdVZN3FbE%40paquier.xyz#eccfd1a0131af93c48026d691c...
It shows that no performance difference for the existing
formats.
The added mechanism may be slower than the existing formats
mechanism but it's not a blocker. Because it's never
performance regression. (Because this is a new feature.)
We can improve it later if it's needed.
> Also, how do we know this API is suitable for the alternative formats?
The v6 patch set has more APIs built on this API. These APIs
are for implementing the alternative formats.
https://www.postgresql.org/message-id/flat/20240124.144936.67229716500876806.kou%40clear-code.com#f1...
This is an Apache Arrow format implementation based on the
v6 patch set: https://github.com/kou/pg-copy-arrow
> For example you mentioned Arrow, and I suppose people will want to add
> support for other column-oriented formats. I assume that will require
> stashing a batch of rows (or some other internal state) somewhere, but
> does the proposed API plan for that?
>
> My guess would be we'll need to add a "private_data" pointer to the
> CopyFromStateData/CopyToStateData structs, but maybe I'm wrong.
I think so too. The v6 patch set has a "private_data"
pointer. But the v17 patch set doesn't have it because the
v17 patch set has only basic changes. We'll add it and other
features in the following patches:
https://www.postgresql.org/message-id/flat/20240305.171808.667980402249336456.kou%40clear-code.com
> I'll send the following patches after this patch is
> merged. They are based on the v6 patch[1]:
>
> 1. Add copy_handler
> * This also adds a pg_proc lookup for custom FORMAT
> * This also adds a test for copy_handler
> 2. Export CopyToStateData
> * We need it to implement custom copy TO handler
> 3. Add needed APIs to implement custom copy TO handler
> * Add CopyToStateData::opaque
> * Export CopySendEndOfRow()
> 4. Export CopyFromStateData
> * We need it to implement custom copy FROM handler
> 5. Add needed APIs to implement custom copy FROM handler
> * Add CopyFromStateData::opaque
> * Export CopyReadBinaryData()
"Copy{To,From}StateDate::opaque" are the "private_data"
pointer in the v6 patch.
> Also, won't the alternative formats require custom parameters. For
> example, for column-oriented-formats it might be useful to specify a
> stripe size (rows per batch), etc. I'm not saying this patch needs to
> implement that, but maybe the API should expect it?
Yes. The v6 patch set also has the API. But we want to
minimize API set as much as possible in the first
implementation.
https://www.postgresql.org/message-id/flat/Zbi1TwPfAvUpKqTd%40paquier.xyz#00abc60c5a1ad9eee395849b7b...
> I am really worried about the complexities
> this thread is getting into because we are trying to shape the
> callbacks in the most generic way possible based on *two* use cases.
> This is going to be a never-ending discussion. I'd rather get some
> simple basics, and then we can discuss if tweaking the callbacks is
> really necessary or not.
And I agree with this approach.
> 1) Switch the existing formats to the new API, to validate the API works
> at least for them, allow testing and benchmarking the code.
I want to keep the current style for the first
implementation to avoid affecting the existing formats
performance. If it's not allowed to move forward this
proposal, could someone help us to solve the mysterious
result (why are %s of CopyOneRowTo() different?) in the
following v15 patch set benchmark result?
https://www.postgresql.org/message-id/flat/ZdbtQJ-p5H1_EDwE%40paquier.xyz#6439e6ad574f2d47cd7220e9bf...
> 2) Try implementing some of the more exotic formats (column-oriented) to
> test the API works for those too.
>
> 3) Maybe try implementing a PoC version to do the DDL, so that it
> actually is extensible.
>
> It's not my intent to "move the goalposts" - I think it's fine if the
> patches (2) and (3) are just PoC, to validate (1) goes in the right
> direction. For example, it's fine if (2) just hard-codes the new format
> next to the build-in ones - that's not something we'd commit, I think,
> but for validation of (1) it's good enough.
>
> Most of the DDL stuff can probably be "copied" from FDW handlers. It's
> pretty similar, and the "routine" idea is what FDW does too. It probably
> also shows a good way to "initialize" the routine, etc.
Is the v6 patch set enough for it?
https://www.postgresql.org/message-id/flat/20240124.144936.67229716500876806.kou%40clear-code.com#f1...
Or should we do it based on the v17 patch set? If so, I'll
work on it now. It was a plan that I'll do after the v17
patch set is merged.
Thanks,
--
kou
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-07-22 08:01 Sutou Kouhei <[email protected]>
parent: Li, Yong <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Sutou Kouhei @ 2024-07-22 08:01 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
Hi Yong,
Thanks for joining this thread!
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 22 Jul 2024 07:11:15 +0000,
"Li, Yong" <[email protected]> wrote:
> My understanding is that the provided v17 patch aims to achieve the followings:
> - Retain existing format implementations as built-in formats, and do not go through the new interface for them.
> - Make sure that there is no sign of performance degradation.
> - Refactoring the existing code to make it easier and possible to make copy handlers extensible. However, some of the infrastructure work that are required to make copy handler extensible are intentionally delayed for future patches. Some of the work were proposed as patches in earlier messages, but they were not explicitly referenced in recent messages.
Right.
Sorry for bothering you. As Tomas suggested, I should have
prepared the current summary.
My last e-mail summarized the current information:
https://www.postgresql.org/message-id/flat/20240722.164540.889091645042390373.kou%40clear-code.com#0...
It also shows that your understanding is right.
Thanks,
--
kou
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-07-22 12:36 Tomas Vondra <[email protected]>
parent: Sutou Kouhei <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Tomas Vondra @ 2024-07-22 12:36 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On 7/22/24 09:45, Sutou Kouhei wrote:
> Hi Tomas,
>
> Thanks for joining this thread!
>
> In <[email protected]>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 19 Jul 2024 14:40:05 +0200,
> Tomas Vondra <[email protected]> wrote:
>
>> I think it'd be helpful if you could post a patch status, i.e. a message
>> re-explaininig what it aims to achieve, summary of the discussion so
>> far, and what you think are the open questions. Otherwise every reviewer
>> has to read the whole thread to learn this.
>
> It makes sense. It seems your questions covers all important
> points in this thread. So my answers of your questions
> summarize the latest information.
>
Thanks for the summary/responses. I still think it'd be better to post a
summary as a separate message, not as yet another post responding to
someone else. If I was reading the thread, I would not have noticed this
is meant to be a summary. I'd even consider putting a "THREAD SUMMARY"
title on the first line, or something like that. Up to you, of course.
As for the patch / decisions, thanks for the responses and explanations.
But I still find it hard to review / make judgements about the approach
based on the current version of the patch :-( Yes, it's entirely
possible earlier versions did something interesting - e.g. it might have
implemented the existing formats to the new approach. Or it might have a
private pointer in v6. But how do I know why it was removed? Was it
because it's unnecessary for the initial version? Or was it because it
turned out to not work?
And when reviewing a patch, I really don't want to scavenge through old
patch versions, looking for random parts. Not only because I don't know
what to look for, but also because it'll be harder and harder to make
those old versions work, as the patch moves evolves.
My suggestions would be to maintain this as a series of patches, making
incremental changes, with the "more complex" or "more experimental"
parts larger in the series. For example, I can imagine doing this:
0001 - minimal version of the patch (e.g. current v17)
0002 - switch existing formats to the new interface
0003 - extend the interface to add bits needed for columnar formats
0004 - add DML to create/alter/drop custom implementations
0005 - minimal patch with extension adding support for Arrow
Or something like that. The idea is that we still have a coherent story
of what we're trying to do, and can discuss the incremental changes
(easier than looking at a large patch). It's even possible to commit
earlier parts before the later parts are quite cleanup up for commit.
And some changes changes may not be even meant for commit (e.g. the
extension) but as guidance / validation for the earlier parts.
I do realize this might look like I'm requiring you to do more work.
Sorry about that. I'm just thinking about how to move the patch forward
and convince myself the approach is OK. Also, it's what I think works
quite well for other patches discussed on this mailing list (I do this
for various patches I submitted, for example). And I'm not even sure it
actually is more work.
As for the performance / profiling issues, I've read the reports and I'm
not sure I see something tremendously wrong. Yes, there are differences,
but 5% change can easily be noise, shift in binary layout, etc.
Unfortunately, there's not much information about what exactly the tests
did, context (hardware, ...). So I don't know, really. But if you share
enough information on how to reproduce this, I'm willing to take a look
and investigate.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2024-07-22 12:36 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-01-17 06:54 [PATCH 4/7] cirrus: code coverage Justin Pryzby <[email protected]>
2024-03-01 05:31 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-03-01 06:44 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-03-04 05:11 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-03-05 06:16 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-03-05 08:18 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-03-06 06:34 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-03-07 06:32 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-03-08 00:22 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-03-11 00:00 ` Re: Make COPY format extendable: Extract COPY TO format implementations jian he <[email protected]>
2024-03-11 00:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-03-13 08:00 ` Re: Make COPY format extendable: Extract COPY TO format implementations jian he <[email protected]>
2024-03-20 14:27 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-04-10 08:16 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-07-19 12:40 ` Re: Make COPY format extendable: Extract COPY TO format implementations Tomas Vondra <[email protected]>
2024-07-22 07:11 ` Re: Make COPY format extendable: Extract COPY TO format implementations Li, Yong <[email protected]>
2024-07-22 08:01 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-07-22 07:45 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-07-22 12:36 ` Re: Make COPY format extendable: Extract COPY TO format implementations Tomas Vondra <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox