public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/7] Extract code to get reason that recovery was stopped to a function.
14+ messages / 4 participants
[nested] [flat]
* [PATCH 3/7] Extract code to get reason that recovery was stopped to a function.
@ 2021-06-21 13:12 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Heikki Linnakangas @ 2021-06-21 13:12 UTC (permalink / raw)
---
src/backend/access/transam/xlog.c | 67 ++++++++++++++++++-------------
1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fb4186ee10d..1e601d6282f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -901,6 +901,7 @@ static void validateRecoveryParameters(void);
static void exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog);
static bool recoveryStopsBefore(XLogReaderState *record);
static bool recoveryStopsAfter(XLogReaderState *record);
+static char *getRecoveryStopReason(void);
static void ConfirmRecoveryPaused(void);
static void recoveryPausesHere(bool endOfRecovery);
static bool recoveryApplyDelay(XLogReaderState *record);
@@ -6059,6 +6060,42 @@ recoveryStopsAfter(XLogReaderState *record)
return false;
}
+/*
+ * Create a comment for the history file to explain why and where
+ * timeline changed.
+ */
+static char *
+getRecoveryStopReason(void)
+{
+ char reason[200];
+
+ if (recoveryTarget == RECOVERY_TARGET_XID)
+ snprintf(reason, sizeof(reason),
+ "%s transaction %u",
+ recoveryStopAfter ? "after" : "before",
+ recoveryStopXid);
+ else if (recoveryTarget == RECOVERY_TARGET_TIME)
+ snprintf(reason, sizeof(reason),
+ "%s %s\n",
+ recoveryStopAfter ? "after" : "before",
+ timestamptz_to_str(recoveryStopTime));
+ else if (recoveryTarget == RECOVERY_TARGET_LSN)
+ snprintf(reason, sizeof(reason),
+ "%s LSN %X/%X\n",
+ recoveryStopAfter ? "after" : "before",
+ LSN_FORMAT_ARGS(recoveryStopLSN));
+ else if (recoveryTarget == RECOVERY_TARGET_NAME)
+ snprintf(reason, sizeof(reason),
+ "at restore point \"%s\"",
+ recoveryStopName);
+ else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
+ snprintf(reason, sizeof(reason), "reached consistency");
+ else
+ snprintf(reason, sizeof(reason), "no recovery target specified");
+
+ return pstrdup(reason);
+}
+
/*
* Wait until shared recoveryPauseState is set to RECOVERY_NOT_PAUSED.
*
@@ -7756,7 +7793,7 @@ StartupXLOG(void)
PrevTimeLineID = ThisTimeLineID;
if (ArchiveRecoveryRequested)
{
- char reason[200];
+ char *reason;
char recoveryPath[MAXPGPATH];
Assert(InArchiveRecovery);
@@ -7765,33 +7802,7 @@ StartupXLOG(void)
ereport(LOG,
(errmsg("selected new timeline ID: %u", ThisTimeLineID)));
- /*
- * Create a comment for the history file to explain why and where
- * timeline changed.
- */
- if (recoveryTarget == RECOVERY_TARGET_XID)
- snprintf(reason, sizeof(reason),
- "%s transaction %u",
- recoveryStopAfter ? "after" : "before",
- recoveryStopXid);
- else if (recoveryTarget == RECOVERY_TARGET_TIME)
- snprintf(reason, sizeof(reason),
- "%s %s\n",
- recoveryStopAfter ? "after" : "before",
- timestamptz_to_str(recoveryStopTime));
- else if (recoveryTarget == RECOVERY_TARGET_LSN)
- snprintf(reason, sizeof(reason),
- "%s LSN %X/%X\n",
- recoveryStopAfter ? "after" : "before",
- LSN_FORMAT_ARGS(recoveryStopLSN));
- else if (recoveryTarget == RECOVERY_TARGET_NAME)
- snprintf(reason, sizeof(reason),
- "at restore point \"%s\"",
- recoveryStopName);
- else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
- snprintf(reason, sizeof(reason), "reached consistency");
- else
- snprintf(reason, sizeof(reason), "no recovery target specified");
+ reason = getRecoveryStopReason();
/*
* We are now done reading the old WAL. Turn off archive fetching if
--
2.30.2
--------------4DE063CB5604E65545208F19
Content-Type: text/x-patch; charset=UTF-8;
name="0004-Move-InRecovery-and-standbyState-global-vars-to-xlog.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0004-Move-InRecovery-and-standbyState-global-vars-to-xlog.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH 3/7] Extract code to get reason that recovery was stopped to a function.
@ 2021-06-21 13:12 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Heikki Linnakangas @ 2021-06-21 13:12 UTC (permalink / raw)
---
src/backend/access/transam/xlog.c | 67 ++++++++++++++++++-------------
1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a491393a336..7c6579cb515 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -891,6 +891,7 @@ static void validateRecoveryParameters(void);
static void exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog);
static bool recoveryStopsBefore(XLogReaderState *record);
static bool recoveryStopsAfter(XLogReaderState *record);
+static char *getRecoveryStopReason(void);
static void ConfirmRecoveryPaused(void);
static void recoveryPausesHere(bool endOfRecovery);
static bool recoveryApplyDelay(XLogReaderState *record);
@@ -6029,6 +6030,42 @@ recoveryStopsAfter(XLogReaderState *record)
return false;
}
+/*
+ * Create a comment for the history file to explain why and where
+ * timeline changed.
+ */
+static char *
+getRecoveryStopReason(void)
+{
+ char reason[200];
+
+ if (recoveryTarget == RECOVERY_TARGET_XID)
+ snprintf(reason, sizeof(reason),
+ "%s transaction %u",
+ recoveryStopAfter ? "after" : "before",
+ recoveryStopXid);
+ else if (recoveryTarget == RECOVERY_TARGET_TIME)
+ snprintf(reason, sizeof(reason),
+ "%s %s\n",
+ recoveryStopAfter ? "after" : "before",
+ timestamptz_to_str(recoveryStopTime));
+ else if (recoveryTarget == RECOVERY_TARGET_LSN)
+ snprintf(reason, sizeof(reason),
+ "%s LSN %X/%X\n",
+ recoveryStopAfter ? "after" : "before",
+ LSN_FORMAT_ARGS(recoveryStopLSN));
+ else if (recoveryTarget == RECOVERY_TARGET_NAME)
+ snprintf(reason, sizeof(reason),
+ "at restore point \"%s\"",
+ recoveryStopName);
+ else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
+ snprintf(reason, sizeof(reason), "reached consistency");
+ else
+ snprintf(reason, sizeof(reason), "no recovery target specified");
+
+ return pstrdup(reason);
+}
+
/*
* Wait until shared recoveryPauseState is set to RECOVERY_NOT_PAUSED.
*
@@ -7718,7 +7755,7 @@ StartupXLOG(void)
PrevTimeLineID = ThisTimeLineID;
if (ArchiveRecoveryRequested)
{
- char reason[200];
+ char *reason;
char recoveryPath[MAXPGPATH];
Assert(InArchiveRecovery);
@@ -7727,33 +7764,7 @@ StartupXLOG(void)
ereport(LOG,
(errmsg("selected new timeline ID: %u", ThisTimeLineID)));
- /*
- * Create a comment for the history file to explain why and where
- * timeline changed.
- */
- if (recoveryTarget == RECOVERY_TARGET_XID)
- snprintf(reason, sizeof(reason),
- "%s transaction %u",
- recoveryStopAfter ? "after" : "before",
- recoveryStopXid);
- else if (recoveryTarget == RECOVERY_TARGET_TIME)
- snprintf(reason, sizeof(reason),
- "%s %s\n",
- recoveryStopAfter ? "after" : "before",
- timestamptz_to_str(recoveryStopTime));
- else if (recoveryTarget == RECOVERY_TARGET_LSN)
- snprintf(reason, sizeof(reason),
- "%s LSN %X/%X\n",
- recoveryStopAfter ? "after" : "before",
- LSN_FORMAT_ARGS(recoveryStopLSN));
- else if (recoveryTarget == RECOVERY_TARGET_NAME)
- snprintf(reason, sizeof(reason),
- "at restore point \"%s\"",
- recoveryStopName);
- else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
- snprintf(reason, sizeof(reason), "reached consistency");
- else
- snprintf(reason, sizeof(reason), "no recovery target specified");
+ reason = getRecoveryStopReason();
/*
* We are now done reading the old WAL. Turn off archive fetching if
--
2.30.2
--------------8C95DA69A2E7D2FC9457CD47
Content-Type: text/x-patch; charset=UTF-8;
name="0004-Move-InRecovery-and-standbyState-global-vars-to-xlog.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0004-Move-InRecovery-and-standbyState-global-vars-to-xlog.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
@ 2024-02-01 01:57 Michael Paquier <[email protected]>
2024-02-01 03:43 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
0 siblings, 2 replies; 14+ messages in thread
From: Michael Paquier @ 2024-02-01 01:57 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Wed, Jan 31, 2024 at 02:39:54PM +0900, Michael Paquier wrote:
> Thanks, I'm looking into that now.
I have much to say about the patch, but for now I have begun running
some performance tests using the patches, because this thread won't
get far until we are sure that the callbacks do not impact performance
in some kind of worst-case scenario. First, here is what I used to
setup a set of tables used for COPY FROM and COPY TO (requires [1] to
feed COPY FROM's data to the void, and note that default values is to
have a strict control on the size of the StringInfos used in the copy
paths):
CREATE EXTENSION blackhole_am;
CREATE OR REPLACE FUNCTION create_table_cols(tabname text, num_cols int)
RETURNS VOID AS
$func$
DECLARE
query text;
BEGIN
query := 'CREATE UNLOGGED TABLE ' || tabname || ' (';
FOR i IN 1..num_cols LOOP
query := query || 'a_' || i::text || ' int default 1';
IF i != num_cols THEN
query := query || ', ';
END IF;
END LOOP;
query := query || ')';
EXECUTE format(query);
END
$func$ LANGUAGE plpgsql;
-- Tables used for COPY TO
SELECT create_table_cols ('to_tab_1', 1);
SELECT create_table_cols ('to_tab_10', 10);
INSERT INTO to_tab_1 SELECT FROM generate_series(1, 10000000);
INSERT INTO to_tab_10 SELECT FROM generate_series(1, 10000000);
-- Data for COPY FROM
COPY to_tab_1 TO '/tmp/to_tab_1.bin' WITH (format binary);
COPY to_tab_10 TO '/tmp/to_tab_10.bin' WITH (format binary);
COPY to_tab_1 TO '/tmp/to_tab_1.txt' WITH (format text);
COPY to_tab_10 TO '/tmp/to_tab_10.txt' WITH (format text);
-- Tables used for COPY FROM
SELECT create_table_cols ('from_tab_1', 1);
SELECT create_table_cols ('from_tab_10', 10);
ALTER TABLE from_tab_1 SET ACCESS METHOD blackhole_am;
ALTER TABLE from_tab_10 SET ACCESS METHOD blackhole_am;
Then I have run a set of tests using HEAD, v7 and v10 with queries
like that (adapt them depending on the format and table):
COPY to_tab_1 TO '/dev/null' WITH (FORMAT text) \watch count=5
SET client_min_messages TO error; -- for blackhole_am
COPY from_tab_1 FROM '/tmp/to_tab_1.txt' with (FORMAT 'text') \watch count=5
COPY from_tab_1 FROM '/tmp/to_tab_1.bin' with (FORMAT 'binary') \watch count=5
All the patches have been compiled with -O2, without assertions, etc.
Postgres is run in tmpfs mode, on scissors, without fsync. Unlogged
tables help a bit in focusing on the execution paths as we don't care
about WAL, of course. I have also included v7 in the test of tests,
as this version uses more simple per-row callbacks.
And here are the results I get for text and binary (ms, average of 15
queries after discarding the three highest and three lowest values):
test | master | v7 | v10
-----------------+--------+------+------
from_bin_1col | 1575 | 1546 | 1575
from_bin_10col | 5364 | 5208 | 5230
from_text_1col | 1690 | 1715 | 1684
from_text_10col | 4875 | 4793 | 4757
to_bin_1col | 1717 | 1730 | 1731
to_bin_10col | 7728 | 7707 | 7513
to_text_1col | 1710 | 1730 | 1698
to_text_10col | 5998 | 5960 | 5987
I am getting an interesting trend here in terms of a speedup between
HEAD and the patches with a table that has 10 attributes filled with
integers, especially for binary and text with COPY FROM. COPY TO
binary also gets nice numbers, while text looks rather stable. Hmm.
These were on my buildfarm animal, but we need to be more confident
about all this. Could more people run these tests? I am going to do
a second session on a local machine I have at hand and see what
happens. Will publish the numbers here, the method will be the same.
[1]: https://github.com/michaelpq/pg_plugins/tree/main/blackhole_am
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-01 03:43 ` Junwang Zhao <[email protected]>
2024-02-01 03:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
1 sibling, 1 reply; 14+ messages in thread
From: Junwang Zhao @ 2024-02-01 03:43 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi Michael,
On Thu, Feb 1, 2024 at 9:58 AM Michael Paquier <[email protected]> wrote:
>
> On Wed, Jan 31, 2024 at 02:39:54PM +0900, Michael Paquier wrote:
> > Thanks, I'm looking into that now.
>
> I have much to say about the patch, but for now I have begun running
> some performance tests using the patches, because this thread won't
> get far until we are sure that the callbacks do not impact performance
> in some kind of worst-case scenario. First, here is what I used to
> setup a set of tables used for COPY FROM and COPY TO (requires [1] to
> feed COPY FROM's data to the void, and note that default values is to
> have a strict control on the size of the StringInfos used in the copy
> paths):
> CREATE EXTENSION blackhole_am;
> CREATE OR REPLACE FUNCTION create_table_cols(tabname text, num_cols int)
> RETURNS VOID AS
> $func$
> DECLARE
> query text;
> BEGIN
> query := 'CREATE UNLOGGED TABLE ' || tabname || ' (';
> FOR i IN 1..num_cols LOOP
> query := query || 'a_' || i::text || ' int default 1';
> IF i != num_cols THEN
> query := query || ', ';
> END IF;
> END LOOP;
> query := query || ')';
> EXECUTE format(query);
> END
> $func$ LANGUAGE plpgsql;
> -- Tables used for COPY TO
> SELECT create_table_cols ('to_tab_1', 1);
> SELECT create_table_cols ('to_tab_10', 10);
> INSERT INTO to_tab_1 SELECT FROM generate_series(1, 10000000);
> INSERT INTO to_tab_10 SELECT FROM generate_series(1, 10000000);
> -- Data for COPY FROM
> COPY to_tab_1 TO '/tmp/to_tab_1.bin' WITH (format binary);
> COPY to_tab_10 TO '/tmp/to_tab_10.bin' WITH (format binary);
> COPY to_tab_1 TO '/tmp/to_tab_1.txt' WITH (format text);
> COPY to_tab_10 TO '/tmp/to_tab_10.txt' WITH (format text);
> -- Tables used for COPY FROM
> SELECT create_table_cols ('from_tab_1', 1);
> SELECT create_table_cols ('from_tab_10', 10);
> ALTER TABLE from_tab_1 SET ACCESS METHOD blackhole_am;
> ALTER TABLE from_tab_10 SET ACCESS METHOD blackhole_am;
>
> Then I have run a set of tests using HEAD, v7 and v10 with queries
> like that (adapt them depending on the format and table):
> COPY to_tab_1 TO '/dev/null' WITH (FORMAT text) \watch count=5
> SET client_min_messages TO error; -- for blackhole_am
> COPY from_tab_1 FROM '/tmp/to_tab_1.txt' with (FORMAT 'text') \watch count=5
> COPY from_tab_1 FROM '/tmp/to_tab_1.bin' with (FORMAT 'binary') \watch count=5
>
> All the patches have been compiled with -O2, without assertions, etc.
> Postgres is run in tmpfs mode, on scissors, without fsync. Unlogged
> tables help a bit in focusing on the execution paths as we don't care
> about WAL, of course. I have also included v7 in the test of tests,
> as this version uses more simple per-row callbacks.
>
> And here are the results I get for text and binary (ms, average of 15
> queries after discarding the three highest and three lowest values):
> test | master | v7 | v10
> -----------------+--------+------+------
> from_bin_1col | 1575 | 1546 | 1575
> from_bin_10col | 5364 | 5208 | 5230
> from_text_1col | 1690 | 1715 | 1684
> from_text_10col | 4875 | 4793 | 4757
> to_bin_1col | 1717 | 1730 | 1731
> to_bin_10col | 7728 | 7707 | 7513
> to_text_1col | 1710 | 1730 | 1698
> to_text_10col | 5998 | 5960 | 5987
>
> I am getting an interesting trend here in terms of a speedup between
> HEAD and the patches with a table that has 10 attributes filled with
> integers, especially for binary and text with COPY FROM. COPY TO
> binary also gets nice numbers, while text looks rather stable. Hmm.
>
> These were on my buildfarm animal, but we need to be more confident
> about all this. Could more people run these tests? I am going to do
> a second session on a local machine I have at hand and see what
> happens. Will publish the numbers here, the method will be the same.
>
> [1]: https://github.com/michaelpq/pg_plugins/tree/main/blackhole_am
> --
> Michael
I'm running the benchmark, but I got some strong numbers:
postgres=# \timing
Timing is on.
postgres=# COPY to_tab_10 TO '/dev/null' WITH (FORMAT binary) \watch count=15
COPY 10000000
Time: 3168.497 ms (00:03.168)
COPY 10000000
Time: 3255.464 ms (00:03.255)
COPY 10000000
Time: 3270.625 ms (00:03.271)
COPY 10000000
Time: 3285.112 ms (00:03.285)
COPY 10000000
Time: 3322.304 ms (00:03.322)
COPY 10000000
Time: 3341.328 ms (00:03.341)
COPY 10000000
Time: 3621.564 ms (00:03.622)
COPY 10000000
Time: 3700.911 ms (00:03.701)
COPY 10000000
Time: 3717.992 ms (00:03.718)
COPY 10000000
Time: 3708.350 ms (00:03.708)
COPY 10000000
Time: 3704.367 ms (00:03.704)
COPY 10000000
Time: 3724.281 ms (00:03.724)
COPY 10000000
Time: 3703.335 ms (00:03.703)
COPY 10000000
Time: 3728.629 ms (00:03.729)
COPY 10000000
Time: 3758.135 ms (00:03.758)
The first 6 rounds are like 10% better than the later 9 rounds, is this normal?
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:43 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
@ 2024-02-01 03:56 ` Michael Paquier <[email protected]>
2024-02-01 04:20 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-02-01 03:56 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Thu, Feb 01, 2024 at 11:43:07AM +0800, Junwang Zhao wrote:
> The first 6 rounds are like 10% better than the later 9 rounds, is this normal?
Even with HEAD? Perhaps you have some OS cache eviction in play here?
FWIW, I'm not seeing any of that with longer runs after 7~ tries in a
loop of 15.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:43 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
2024-02-01 03:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-01 04:20 ` Junwang Zhao <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Junwang Zhao @ 2024-02-01 04:20 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Thu, Feb 1, 2024 at 11:56 AM Michael Paquier <[email protected]> wrote:
>
> On Thu, Feb 01, 2024 at 11:43:07AM +0800, Junwang Zhao wrote:
> > The first 6 rounds are like 10% better than the later 9 rounds, is this normal?
>
> Even with HEAD? Perhaps you have some OS cache eviction in play here?
> FWIW, I'm not seeing any of that with longer runs after 7~ tries in a
> loop of 15.
Yeah, with HEAD. I'm on ubuntu 22.04, I did not change any gucs, maybe I should
set a higher shared_buffers? But I dought that's related ;(
> --
> Michael
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-01 03:49 ` Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
1 sibling, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-02-01 03:49 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Thu, Feb 01, 2024 at 10:57:58AM +0900, Michael Paquier wrote:
> And here are the results I get for text and binary (ms, average of 15
> queries after discarding the three highest and three lowest values):
> test | master | v7 | v10
> -----------------+--------+------+------
> from_bin_1col | 1575 | 1546 | 1575
> from_bin_10col | 5364 | 5208 | 5230
> from_text_1col | 1690 | 1715 | 1684
> from_text_10col | 4875 | 4793 | 4757
> to_bin_1col | 1717 | 1730 | 1731
> to_bin_10col | 7728 | 7707 | 7513
> to_text_1col | 1710 | 1730 | 1698
> to_text_10col | 5998 | 5960 | 5987
Here are some numbers from a second local machine:
test | master | v7 | v10
-----------------+--------+------+------
from_bin_1col | 508 | 467 | 461
from_bin_10col | 2192 | 2083 | 2098
from_text_1col | 510 | 499 | 517
from_text_10col | 1970 | 1678 | 1654
to_bin_1col | 575 | 577 | 573
to_bin_10col | 2680 | 2678 | 2722
to_text_1col | 516 | 506 | 527
to_text_10col | 2250 | 2245 | 2235
This is confirming a speedup with COPY FROM for both text and binary,
with more impact with a larger number of attributes. That's harder to
conclude about COPY TO in both cases, but at least I'm not seeing any
regression even with some variance caused by what looks like noise.
We need more numbers from more people. Sutou-san or Sawada-san, or
any volunteers?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-01 15:19 ` Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Sutou Kouhei @ 2024-02-01 15:19 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
Thanks for preparing benchmark.
In <[email protected]>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 1 Feb 2024 12:49:59 +0900,
Michael Paquier <[email protected]> wrote:
> On Thu, Feb 01, 2024 at 10:57:58AM +0900, Michael Paquier wrote:
>> And here are the results I get for text and binary (ms, average of 15
>> queries after discarding the three highest and three lowest values):
>> test | master | v7 | v10
>> -----------------+--------+------+------
>> from_bin_1col | 1575 | 1546 | 1575
>> from_bin_10col | 5364 | 5208 | 5230
>> from_text_1col | 1690 | 1715 | 1684
>> from_text_10col | 4875 | 4793 | 4757
>> to_bin_1col | 1717 | 1730 | 1731
>> to_bin_10col | 7728 | 7707 | 7513
>> to_text_1col | 1710 | 1730 | 1698
>> to_text_10col | 5998 | 5960 | 5987
>
> Here are some numbers from a second local machine:
> test | master | v7 | v10
> -----------------+--------+------+------
> from_bin_1col | 508 | 467 | 461
> from_bin_10col | 2192 | 2083 | 2098
> from_text_1col | 510 | 499 | 517
> from_text_10col | 1970 | 1678 | 1654
> to_bin_1col | 575 | 577 | 573
> to_bin_10col | 2680 | 2678 | 2722
> to_text_1col | 516 | 506 | 527
> to_text_10col | 2250 | 2245 | 2235
>
> This is confirming a speedup with COPY FROM for both text and binary,
> with more impact with a larger number of attributes. That's harder to
> conclude about COPY TO in both cases, but at least I'm not seeing any
> regression even with some variance caused by what looks like noise.
> We need more numbers from more people. Sutou-san or Sawada-san, or
> any volunteers?
Here are some numbers on my local machine (Note that my
local machine isn't suitable for benchmark as I said
before. Each number is median of "\watch 15" results):
1:
direction format n_columns master v7 v10
to text 1 1077.254 1016.953 1028.434
to csv 1 1079.88 1055.545 1053.95
to binary 1 1051.247 1033.93 1003.44
to text 10 4373.168 3980.442 3955.94
to csv 10 4753.842 4719.2 4677.643
to binary 10 4598.374 4431.238 4285.757
from text 1 875.729 916.526 869.283
from csv 1 909.355 1001.277 918.655
from binary 1 872.943 907.778 859.433
from text 10 2594.429 2345.292 2587.603
from csv 10 2968.972 3039.544 2964.468
from binary 10 3072.01 3109.267 3093.983
2:
direction format n_columns master v7 v10
to text 1 1061.908 988.768 978.291
to csv 1 1095.109 1037.015 1041.613
to binary 1 1076.992 1000.212 983.318
to text 10 4336.517 3901.833 3841.789
to csv 10 4679.411 4640.975 4570.774
to binary 10 4465.04 4508.063 4261.749
from text 1 866.689 917.54 830.417
from csv 1 917.973 1695.401 871.991
from binary 1 841.104 1422.012 820.786
from text 10 2523.607 3147.738 2517.505
from csv 10 2917.018 3042.685 2950.338
from binary 10 2998.051 3128.542 3018.954
3:
direction format n_columns master v7 v10
to text 1 1021.168 1031.183 962.945
to csv 1 1076.549 1069.661 1060.258
to binary 1 1024.611 1022.143 975.768
to text 10 4327.24 3936.703 4049.893
to csv 10 4620.436 4531.676 4685.672
to binary 10 4457.165 4390.992 4301.463
from text 1 887.532 907.365 888.892
from csv 1 945.167 1012.29 895.921
from binary 1 853.06 854.652 849.661
from text 10 2660.509 2304.256 2527.071
from csv 10 2913.644 2968.204 2935.081
from binary 10 3020.812 3081.162 3090.803
I'll measure again on my local machine later. I'll stop
other processes such as Web browser, editor and so on as
much as possible when I do.
Thanks,
--
kou
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
@ 2024-02-01 21:51 ` Michael Paquier <[email protected]>
2024-02-02 00:40 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-02 00:52 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
0 siblings, 2 replies; 14+ messages in thread
From: Michael Paquier @ 2024-02-01 21:51 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Fri, Feb 02, 2024 at 12:19:51AM +0900, Sutou Kouhei wrote:
> Here are some numbers on my local machine (Note that my
> local machine isn't suitable for benchmark as I said
> before. Each number is median of "\watch 15" results):
>>
> I'll measure again on my local machine later. I'll stop
> other processes such as Web browser, editor and so on as
> much as possible when I do.
Thanks for compiling some numbers. This is showing a lot of variance.
Expecially, these two lines in table 2 are showing surprising results
for v7:
direction format n_columns master v7 v10
from csv 1 917.973 1695.401 871.991
from binary 1 841.104 1422.012 820.786
I am going to try to plug in some rusage() calls in the backend for
the COPY paths. I hope that gives more precision about the backend
activity. I'll post that with more numbers.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-02 00:40 ` Sutou Kouhei <[email protected]>
2024-02-02 06:21 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
1 sibling, 1 reply; 14+ messages in thread
From: Sutou Kouhei @ 2024-02-02 00:40 UTC (permalink / raw)
To: [email protected]; +Cc: [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, 2 Feb 2024 06:51:02 +0900,
Michael Paquier <[email protected]> wrote:
> On Fri, Feb 02, 2024 at 12:19:51AM +0900, Sutou Kouhei wrote:
>> Here are some numbers on my local machine (Note that my
>> local machine isn't suitable for benchmark as I said
>> before. Each number is median of "\watch 15" results):
>>>
>> I'll measure again on my local machine later. I'll stop
>> other processes such as Web browser, editor and so on as
>> much as possible when I do.
>
> Thanks for compiling some numbers. This is showing a lot of variance.
> Expecially, these two lines in table 2 are showing surprising results
> for v7:
> direction format n_columns master v7 v10
> from csv 1 917.973 1695.401 871.991
> from binary 1 841.104 1422.012 820.786
Here are more numbers:
1:
direction format n_columns master v7 v10
to text 1 1053.844 978.998 956.575
to csv 1 1091.316 1020.584 1098.314
to binary 1 1034.685 969.224 980.458
to text 10 4216.264 3886.515 4111.417
to csv 10 4649.228 4530.882 4682.988
to binary 10 4219.228 4189.99 4211.942
from text 1 851.697 896.968 890.458
from csv 1 890.229 936.231 887.15
from binary 1 784.407 817.07 938.736
from text 10 2549.056 2233.899 2630.892
from csv 10 2809.441 2868.411 2895.196
from binary 10 2985.674 3027.522 3397.5
2:
direction format n_columns master v7 v10
to text 1 1013.764 1011.968 940.855
to csv 1 1060.431 1065.468 1040.68
to binary 1 1013.652 1009.956 965.675
to text 10 4411.484 4031.571 3896.836
to csv 10 4739.625 4715.81 4631.002
to binary 10 4374.077 4357.942 4227.215
from text 1 955.078 922.346 866.222
from csv 1 1040.717 986.524 905.657
from binary 1 849.316 864.859 833.152
from text 10 2703.209 2361.651 2533.992
from csv 10 2990.35 3059.167 2930.632
from binary 10 3008.375 3368.714 3055.723
3:
direction format n_columns master v7 v10
to text 1 1084.756 1003.822 994.409
to csv 1 1092.4 1062.536 1079.027
to binary 1 1046.774 994.168 993.633
to text 10 4363.51 3978.205 4124.359
to csv 10 4866.762 4616.001 4715.052
to binary 10 4382.412 4363.269 4213.456
from text 1 852.976 907.315 860.749
from csv 1 925.187 962.632 897.833
from binary 1 824.997 897.046 828.231
from text 10 2591.07 2358.541 2540.431
from csv 10 2907.033 3018.486 2915.997
from binary 10 3069.027 3209.21 3119.128
Other processes are stopped while I measure them. But I'm
not sure these numbers are more reliable than before...
> I am going to try to plug in some rusage() calls in the backend for
> the COPY paths. I hope that gives more precision about the backend
> activity. I'll post that with more numbers.
Thanks. It'll help us.
--
kou
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-02 00:40 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
@ 2024-02-02 06:21 ` Michael Paquier <[email protected]>
2024-02-02 07:27 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Michael Paquier @ 2024-02-02 06:21 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Fri, Feb 02, 2024 at 09:40:56AM +0900, Sutou Kouhei wrote:
> Thanks. It'll help us.
I have done a review of v10, see v11 attached which is still WIP, with
the patches for COPY TO and COPY FROM merged together. Note that I'm
thinking to merge them into a single commit.
@@ -74,11 +75,11 @@ typedef struct CopyFormatOptions
bool convert_selectively; /* do selective binary conversion? */
CopyOnErrorChoice on_error; /* what to do when error happened */
List *convert_select; /* list of column names (can be NIL) */
+ const CopyToRoutine *to_routine; /* callback routines for COPY TO */
} CopyFormatOptions;
Adding the routines to the structure for the format options is in my
opinion incorrect. The elements of this structure are first processed
in the option deparsing path, and then we need to use the options to
guess which routines we need. A more natural location is cstate
itself, so as the pointer to the routines is isolated within copyto.c
and copyfrom_internal.h. My point is: the routines are an
implementation detail that the centralized copy.c has no need to know
about. This also led to a strange separation with
ProcessCopyOptionFormatFrom() and ProcessCopyOptionFormatTo() to fit
the hole in-between.
The separation between cstate and the format-related fields could be
much better, though I am not sure if it is worth doing as it
introduces more duplication. For example, max_fields and raw_fields
are specific to text and csv, while binary does not care much.
Perhaps this is just useful to be for custom formats.
copyapi.h needs more documentation, like what is expected for
extension developers when using these, what are the arguments, etc. I
have added what I had in mind for now.
+typedef char *(*PostpareColumnValue) (CopyFromState cstate, char *string, int m);
CopyReadAttributes and PostpareColumnValue are also callbacks specific
to text and csv, except that they are used within the per-row
callbacks. The same can be said about CopyAttributeOutHeaderFunction.
It seems to me that it would be less confusing to store pointers to
them in the routine structures, where the final picture involves not
having multiple layers of APIs like CopyToCSVStart,
CopyAttributeOutTextValue, etc. These *have* to be documented
properly in copyapi.h, and this is much easier now that cstate stores
the routine pointers. That would also make simpler function stacks.
Note that I have not changed that in the v11 attached.
This business with the extra callbacks required for csv and text is my
main point of contention, but I'd be OK once the model of the APIs is
more linear, with everything in Copy{From,To}State. The changes would
be rather simple, and I'd be OK to put my hands on it. Just,
Sutou-san, would you agree with my last point about these extra
callbacks?
--
Michael
Attachments:
[text/x-diff] v11-0001-Extract-COPY-FROM-TO-format-implementations.patch (43.8K, ../../[email protected]/2-v11-0001-Extract-COPY-FROM-TO-format-implementations.patch)
download | inline diff:
From 7aca58d0f7adfd146d287059b1d11b47acdfa758 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email protected]>
Date: Wed, 31 Jan 2024 13:37:02 +0900
Subject: [PATCH v11] Extract COPY FROM/TO format implementations
This doesn't change the current behavior. This just introduces a set of
copy routines called CopyFromRoutine and CopyToRoutine, which just has
function pointers of format implementation like TupleTableSlotOps, and
use it for existing "text", "csv" and "binary" format implementations.
There are plans to extend that more in the future with custom formats.
This improves performance when a relation has many attributes as this
eliminates format-based if branches. The more the attributes, the
better the performance gain. Blah add some numbers.
---
src/include/commands/copyapi.h | 82 ++++
src/include/commands/copyfrom_internal.h | 8 +
src/backend/commands/copyfrom.c | 219 +++++++---
src/backend/commands/copyfromparse.c | 439 ++++++++++++--------
src/backend/commands/copyto.c | 496 ++++++++++++++++-------
src/tools/pgindent/typedefs.list | 2 +
6 files changed, 870 insertions(+), 376 deletions(-)
create mode 100644 src/include/commands/copyapi.h
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
new file mode 100644
index 0000000000..3c0a6ba7a1
--- /dev/null
+++ b/src/include/commands/copyapi.h
@@ -0,0 +1,82 @@
+/*-------------------------------------------------------------------------
+ *
+ * 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.
+ *
+ * `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, including setting cstate->in_functions and/or
+ * cstate->typioparams.
+ */
+ 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.
+ *
+ * `tupDesc` is the tuple descriptor of the relation from where the
+ * data is read. This can be used for any initialization steps required
+ * by a format, including setting cstate->out_functions.
+ */
+ 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..d7de2d9d11 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 */
@@ -183,4 +187,8 @@ typedef struct CopyFromStateData
extern void ReceiveCopyBegin(CopyFromState cstate);
extern void ReceiveCopyBinaryHeader(CopyFromState cstate);
+extern bool CopyFromTextOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls);
+extern bool CopyFromCSVOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls);
+extern bool CopyFromBinaryOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls);
+
#endif /* COPYFROM_INTERNAL_H */
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 1fe70b9133..cad6f04182 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -108,6 +108,170 @@ static char *limit_printout_length(const char *str);
static void ClosePipeFromProgram(CopyFromState cstate);
+
+/*
+ * CopyFromRoutine implementations.
+ */
+
+/*
+ * CopyFromRoutine implementation for "text" and "csv". CopyFromTextBased*()
+ * are shared by both of "text" and "csv". CopyFromText*() are only for "text"
+ * and CopyFromCSV*() are only for "csv".
+ *
+ * We can use the same functions for all callbacks by referring
+ * cstate->opts.csv_mode but splitting callbacks to eliminate "if
+ * (cstate->opts.csv_mode)" branches from all callbacks has performance merit
+ * when many tuples are copied. So we use separated callbacks for "text" and
+ * "csv".
+ */
+
+/*
+ * This must initialize cstate->in_functions for CopyFromTextBasedOneRow().
+ */
+static void
+CopyFromTextStart(CopyFromState cstate, TupleDesc tupDesc)
+{
+ AttrNumber num_phys_attrs = tupDesc->natts;
+ AttrNumber attr_count;
+
+ /*
+ * If encoding conversion is needed, we need another buffer to hold the
+ * converted input data. Otherwise, we can just point input_buf to the
+ * same buffer as raw_buf.
+ */
+ if (cstate->need_transcoding)
+ {
+ cstate->input_buf = (char *) palloc(INPUT_BUF_SIZE + 1);
+ cstate->input_buf_index = cstate->input_buf_len = 0;
+ }
+ else
+ cstate->input_buf = cstate->raw_buf;
+ cstate->input_reached_eof = false;
+
+ initStringInfo(&cstate->line_buf);
+
+ /*
+ * Assign the required catalog information for each attribute in the
+ * relation, including the input function and the element type (to pass
+ * to the input function).
+ */
+ cstate->in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
+ cstate->typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
+
+ for (int attnum = 1; attnum <= num_phys_attrs; attnum++)
+ {
+ Form_pg_attribute att = TupleDescAttr(tupDesc, attnum - 1);
+ Oid in_func_oid;
+
+ /* We don't need info for dropped attributes */
+ if (att->attisdropped)
+ continue;
+
+ /* Fetch the input function and typioparam info */
+ getTypeInputInfo(att->atttypid,
+ &in_func_oid, &cstate->typioparams[attnum - 1]);
+ fmgr_info(in_func_oid, &cstate->in_functions[attnum - 1]);
+ }
+
+ /* create workspace for CopyReadAttributes results */
+ attr_count = list_length(cstate->attnumlist);
+ cstate->max_fields = attr_count;
+ cstate->raw_fields = (char **) palloc(attr_count * sizeof(char *));
+}
+
+static void
+CopyFromTextEnd(CopyFromState cstate)
+{
+ /* nothing to do */
+}
+
+/*
+ * CopyFromRoutine implementation for "binary".
+ */
+
+/*
+ * CopyFromBinaryStart
+ *
+ * This must initialize cstate->in_functions for CopyFromBinaryOneRow().
+ */
+static void
+CopyFromBinaryStart(CopyFromState cstate, TupleDesc tupDesc)
+{
+ AttrNumber num_phys_attrs = tupDesc->natts;
+
+ /*
+ * Assign the required catalog information for each attribute in the
+ * relation, including the input function and the element type (to pass
+ * to the input function).
+ */
+ cstate->in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
+ cstate->typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
+
+ for (int attnum = 1; attnum <= num_phys_attrs; attnum++)
+ {
+ Form_pg_attribute att = TupleDescAttr(tupDesc, attnum - 1);
+ Oid in_func_oid;
+
+ /* We don't need info for dropped attributes */
+ if (att->attisdropped)
+ continue;
+
+ /* Fetch the input function and typioparam info */
+ getTypeBinaryInputInfo(att->atttypid,
+ &in_func_oid, &cstate->typioparams[attnum - 1]);
+ fmgr_info(in_func_oid, &cstate->in_functions[attnum - 1]);
+ }
+
+ /* Read and verify binary header */
+ ReceiveCopyBinaryHeader(cstate);
+}
+
+static void
+CopyFromBinaryEnd(CopyFromState cstate)
+{
+ /* nothing to do */
+}
+
+/*
+ * Callback routines assigned to each format.
+ *
+ * Note that start and end callbacks are shared for CSV and text, while
+ * the per-row callback is kept separated.
+ */
+static const CopyFromRoutine CopyFromRoutineText = {
+ .CopyFromStart = CopyFromTextStart,
+ .CopyFromOneRow = CopyFromTextOneRow,
+ .CopyFromEnd = CopyFromTextEnd,
+};
+
+static const CopyFromRoutine CopyFromRoutineCSV = {
+ .CopyFromStart = CopyFromTextStart,
+ .CopyFromOneRow = CopyFromCSVOneRow,
+ .CopyFromEnd = CopyFromTextEnd,
+};
+
+static const CopyFromRoutine CopyFromRoutineBinary = {
+ .CopyFromStart = CopyFromBinaryStart,
+ .CopyFromOneRow = CopyFromBinaryOneRow,
+ .CopyFromEnd = CopyFromBinaryEnd,
+};
+
+/*
+ * Define the COPY FROM routines to use for a format.
+ */
+static const CopyFromRoutine *
+CopyFromGetRoutine(CopyFormatOptions opts)
+{
+ if (opts.csv_mode)
+ return &CopyFromRoutineCSV;
+ else if (opts.binary)
+ return &CopyFromRoutineBinary;
+
+ /* default is text */
+ return &CopyFromRoutineText;
+}
+
+
/*
* error context callback for COPY FROM
*
@@ -1384,9 +1548,6 @@ BeginCopyFrom(ParseState *pstate,
TupleDesc tupDesc;
AttrNumber num_phys_attrs,
num_defaults;
- FmgrInfo *in_functions;
- Oid *typioparams;
- Oid in_func_oid;
int *defmap;
ExprState **defexprs;
MemoryContext oldcontext;
@@ -1418,6 +1579,9 @@ BeginCopyFrom(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
+ /* Set format routine */
+ cstate->routine = CopyFromGetRoutine(cstate->opts);
+
/* Process the target relation */
cstate->rel = rel;
@@ -1571,25 +1735,6 @@ BeginCopyFrom(ParseState *pstate,
cstate->raw_buf_index = cstate->raw_buf_len = 0;
cstate->raw_reached_eof = false;
- if (!cstate->opts.binary)
- {
- /*
- * If encoding conversion is needed, we need another buffer to hold
- * the converted input data. Otherwise, we can just point input_buf
- * to the same buffer as raw_buf.
- */
- if (cstate->need_transcoding)
- {
- cstate->input_buf = (char *) palloc(INPUT_BUF_SIZE + 1);
- cstate->input_buf_index = cstate->input_buf_len = 0;
- }
- else
- cstate->input_buf = cstate->raw_buf;
- cstate->input_reached_eof = false;
-
- initStringInfo(&cstate->line_buf);
- }
-
initStringInfo(&cstate->attribute_buf);
/* Assign range table and rteperminfos, we'll need them in CopyFrom. */
@@ -1608,8 +1753,6 @@ BeginCopyFrom(ParseState *pstate,
* the input function), and info about defaults and constraints. (Which
* input function we use depends on text/binary format choice.)
*/
- in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
- typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
defmap = (int *) palloc(num_phys_attrs * sizeof(int));
defexprs = (ExprState **) palloc(num_phys_attrs * sizeof(ExprState *));
@@ -1621,15 +1764,6 @@ BeginCopyFrom(ParseState *pstate,
if (att->attisdropped)
continue;
- /* Fetch the input function and typioparam info */
- if (cstate->opts.binary)
- getTypeBinaryInputInfo(att->atttypid,
- &in_func_oid, &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;
@@ -1689,8 +1823,6 @@ BeginCopyFrom(ParseState *pstate,
cstate->bytes_processed = 0;
/* We keep those variables in cstate. */
- cstate->in_functions = in_functions;
- cstate->typioparams = typioparams;
cstate->defmap = defmap;
cstate->defexprs = defexprs;
cstate->volatile_defexprs = volatile_defexprs;
@@ -1763,20 +1895,7 @@ BeginCopyFrom(ParseState *pstate,
pgstat_progress_update_multi_param(3, progress_cols, progress_vals);
- if (cstate->opts.binary)
- {
- /* Read and verify binary header */
- ReceiveCopyBinaryHeader(cstate);
- }
-
- /* create workspace for CopyReadAttributes results */
- if (!cstate->opts.binary)
- {
- AttrNumber attr_count = list_length(cstate->attnumlist);
-
- cstate->max_fields = attr_count;
- cstate->raw_fields = (char **) palloc(attr_count * sizeof(char *));
- }
+ cstate->routine->CopyFromStart(cstate, tupDesc);
MemoryContextSwitchTo(oldcontext);
@@ -1789,6 +1908,8 @@ BeginCopyFrom(ParseState *pstate,
void
EndCopyFrom(CopyFromState cstate)
{
+ 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..bac8b0b250 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -740,8 +740,19 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
return copied_bytes;
}
+typedef int (*CopyReadAttributes) (CopyFromState cstate);
+
/*
- * Read raw fields in the next line for COPY FROM in text or csv mode.
+ * Read raw fields in the next line for COPY FROM in text or csv
+ * mode. CopyReadAttributesText() must be used for text mode and
+ * CopyReadAttributesCSV() for csv mode. This inconvenient is for
+ * optimization. If "if (cstate->opts.csv_mode)" branch is removed, there is
+ * performance merit for COPY FROM with many tuples.
+ *
+ * NextCopyFromRawFields() can be used instead for convenience
+ * use. NextCopyFromRawFields() chooses CopyReadAttributesText() or
+ * CopyReadAttributesCSV() internally.
+ *
* Return false if no more lines.
*
* An internal temporary buffer is returned via 'fields'. It is valid until
@@ -751,8 +762,10 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
*
* NOTE: force_not_null option are not applied to the returned fields.
*/
-bool
-NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
+static inline bool
+NextCopyFromRawFieldsInternal(CopyFromState cstate,
+ char ***fields, int *nfields,
+ CopyReadAttributes copy_read_attributes)
{
int fldct;
bool done;
@@ -775,11 +788,7 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
{
int fldnum;
- if (cstate->opts.csv_mode)
- fldct = CopyReadAttributesCSV(cstate);
- else
- fldct = CopyReadAttributesText(cstate);
-
+ fldct = copy_read_attributes(cstate);
if (fldct != list_length(cstate->attnumlist))
ereport(ERROR,
(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
@@ -830,16 +839,254 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
return false;
/* Parse the line into de-escaped field values */
- if (cstate->opts.csv_mode)
- fldct = CopyReadAttributesCSV(cstate);
- else
- fldct = CopyReadAttributesText(cstate);
+ fldct = copy_read_attributes(cstate);
*fields = cstate->raw_fields;
*nfields = fldct;
return true;
}
+/*
+ * See NextCopyFromRawFieldsInternal() for details.
+ */
+bool
+NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
+{
+ if (cstate->opts.csv_mode)
+ return NextCopyFromRawFieldsInternal(cstate, fields, nfields, CopyReadAttributesCSV);
+ else
+ return NextCopyFromRawFieldsInternal(cstate, fields, nfields, CopyReadAttributesText);
+}
+
+typedef char *(*PostpareColumnValue) (CopyFromState cstate, char *string, int m);
+
+static inline char *
+PostpareColumnValueText(CopyFromState cstate, char *string, int m)
+{
+ /* do nothing */
+ return string;
+}
+
+static inline char *
+PostpareColumnValueCSV(CopyFromState cstate, char *string, int m)
+{
+ if (string == NULL &&
+ cstate->opts.force_notnull_flags[m])
+ {
+ /*
+ * FORCE_NOT_NULL option is set and column is NULL - convert it to the
+ * NULL string.
+ */
+ string = cstate->opts.null_print;
+ }
+ else if (string != NULL && cstate->opts.force_null_flags[m]
+ && strcmp(string, cstate->opts.null_print) == 0)
+ {
+ /*
+ * FORCE_NULL option is set and column matches the NULL string. It
+ * must have been quoted, or otherwise the string would already have
+ * been set to NULL. Convert it to NULL as specified.
+ */
+ string = NULL;
+ }
+ return string;
+}
+
+/*
+ * CopyFromTextBasedOneRow
+ *
+ * Workhorse for CopyFromTextOneRow() and CopyFromCSVOneRow().
+ *
+ * This function is not directly used as a callback; it is CopyFromTextOneRow()
+ * and CopyFromCSVOneRow()'s responsibility to do that instead. This eliminates
+ * "if" branches with csv_mode, which is an optimization that matters for
+ * performance as these callbacks are called once per tuple.
+ */
+static inline bool
+CopyFromTextBasedOneRow(CopyFromState cstate,
+ ExprContext *econtext,
+ Datum *values,
+ bool *nulls,
+ CopyReadAttributes copy_read_attributes,
+ PostpareColumnValue postpare_column_value)
+{
+ TupleDesc tupDesc;
+ AttrNumber attr_count;
+ FmgrInfo *in_functions = cstate->in_functions;
+ Oid *typioparams = cstate->typioparams;
+ ExprState **defexprs = cstate->defexprs;
+ char **field_strings;
+ ListCell *cur;
+ int fldct;
+ int fieldno;
+ char *string;
+
+ tupDesc = RelationGetDescr(cstate->rel);
+ attr_count = list_length(cstate->attnumlist);
+
+ /* read raw fields in the next line */
+ if (!NextCopyFromRawFieldsInternal(cstate, &field_strings,
+ &fldct, copy_read_attributes))
+ return false;
+
+ /* check for overflowing fields */
+ if (attr_count > 0 && fldct > attr_count)
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("extra data after last expected column")));
+
+ fieldno = 0;
+
+ /* Loop to read the user attributes on the line. */
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ int m = attnum - 1;
+ Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+
+ if (fieldno >= fldct)
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("missing data for column \"%s\"",
+ NameStr(att->attname))));
+ string = field_strings[fieldno++];
+
+ if (cstate->convert_select_flags &&
+ !cstate->convert_select_flags[m])
+ {
+ /* ignore input field, leaving column as NULL */
+ continue;
+ }
+
+ cstate->cur_attname = NameStr(att->attname);
+ cstate->cur_attval = string;
+
+ string = postpare_column_value(cstate, string, m);
+
+ if (string != NULL)
+ nulls[m] = false;
+
+ if (cstate->defaults[m])
+ {
+ /*
+ * The caller must supply econtext and have switched into the
+ * per-tuple memory context in it.
+ */
+ Assert(econtext != NULL);
+ Assert(CurrentMemoryContext == econtext->ecxt_per_tuple_memory);
+
+ values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
+ }
+
+ /*
+ * If ON_ERROR is specified with IGNORE, skip rows with soft errors
+ */
+ else if (!InputFunctionCallSafe(&in_functions[m],
+ string,
+ typioparams[m],
+ att->atttypmod,
+ (Node *) cstate->escontext,
+ &values[m]))
+ {
+ cstate->num_errors++;
+ return true;
+ }
+
+ cstate->cur_attname = NULL;
+ cstate->cur_attval = NULL;
+ }
+
+ Assert(fieldno == attr_count);
+
+ return true;
+}
+
+
+bool
+CopyFromTextOneRow(CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls)
+{
+ return CopyFromTextBasedOneRow(cstate, econtext, values, nulls,
+ CopyReadAttributesText,
+ PostpareColumnValueText);
+}
+
+bool
+CopyFromCSVOneRow(CopyFromState cstate, ExprContext *econtext,
+ Datum *values, bool *nulls)
+{
+ return CopyFromTextBasedOneRow(cstate, econtext, values, nulls,
+ CopyReadAttributesCSV,
+ PostpareColumnValueCSV);
+}
+
+/*
+ * cstate->in_functions must be initialized in CopyFromBinaryStart().
+ */
+bool
+CopyFromBinaryOneRow(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls)
+{
+ TupleDesc tupDesc;
+ AttrNumber attr_count;
+ FmgrInfo *in_functions = cstate->in_functions;
+ Oid *typioparams = cstate->typioparams;
+ int16 fld_count;
+ ListCell *cur;
+
+ tupDesc = RelationGetDescr(cstate->rel);
+ attr_count = list_length(cstate->attnumlist);
+
+ cstate->cur_lineno++;
+
+ if (!CopyGetInt16(cstate, &fld_count))
+ {
+ /* EOF detected (end of file, or protocol-level EOF) */
+ return false;
+ }
+
+ if (fld_count == -1)
+ {
+ /*
+ * Received EOF marker. Wait for the protocol-level EOF, and complain
+ * if it doesn't come immediately. In COPY FROM STDIN, this ensures
+ * that we correctly handle CopyFail, if client chooses to send that
+ * now. When copying from file, we could ignore the rest of the file
+ * like in text mode, but we choose to be consistent with the COPY
+ * FROM STDIN case.
+ */
+ char dummy;
+
+ if (CopyReadBinaryData(cstate, &dummy, 1) > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("received copy data after EOF marker")));
+ return false;
+ }
+
+ if (fld_count != attr_count)
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("row field count is %d, expected %d",
+ (int) fld_count, attr_count)));
+
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ int m = attnum - 1;
+ Form_pg_attribute att = TupleDescAttr(tupDesc, m);
+
+ cstate->cur_attname = NameStr(att->attname);
+ values[m] = CopyReadBinaryAttribute(cstate,
+ &in_functions[m],
+ typioparams[m],
+ att->atttypmod,
+ &nulls[m]);
+ cstate->cur_attname = NULL;
+ }
+
+ return true;
+}
+
/*
* Read next tuple from file for COPY FROM. Return false if no more tuples.
*
@@ -849,7 +1096,8 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
* read from the file, and DEFAULT option is unset.
*
* 'values' and 'nulls' arrays must be the same length as columns of the
- * relation passed to BeginCopyFrom. This function fills the arrays.
+ * relation passed to BeginCopyFrom. This function calls the format routine
+ * that should fill the arrays.
*/
bool
NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
@@ -857,181 +1105,22 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
{
TupleDesc tupDesc;
AttrNumber num_phys_attrs,
- attr_count,
num_defaults = cstate->num_defaults;
- FmgrInfo *in_functions = cstate->in_functions;
- Oid *typioparams = cstate->typioparams;
int i;
int *defmap = cstate->defmap;
ExprState **defexprs = cstate->defexprs;
tupDesc = RelationGetDescr(cstate->rel);
num_phys_attrs = tupDesc->natts;
- attr_count = list_length(cstate->attnumlist);
/* Initialize all values for row to NULL */
MemSet(values, 0, num_phys_attrs * sizeof(Datum));
MemSet(nulls, true, num_phys_attrs * sizeof(bool));
MemSet(cstate->defaults, false, num_phys_attrs * sizeof(bool));
- if (!cstate->opts.binary)
- {
- char **field_strings;
- ListCell *cur;
- int fldct;
- int fieldno;
- char *string;
-
- /* read raw fields in the next line */
- if (!NextCopyFromRawFields(cstate, &field_strings, &fldct))
- return false;
-
- /* check for overflowing fields */
- if (attr_count > 0 && fldct > attr_count)
- ereport(ERROR,
- (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
- errmsg("extra data after last expected column")));
-
- fieldno = 0;
-
- /* Loop to read the user attributes on the line. */
- foreach(cur, cstate->attnumlist)
- {
- int attnum = lfirst_int(cur);
- int m = attnum - 1;
- Form_pg_attribute att = TupleDescAttr(tupDesc, m);
-
- if (fieldno >= fldct)
- ereport(ERROR,
- (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
- errmsg("missing data for column \"%s\"",
- NameStr(att->attname))));
- string = field_strings[fieldno++];
-
- if (cstate->convert_select_flags &&
- !cstate->convert_select_flags[m])
- {
- /* ignore input field, leaving column as NULL */
- continue;
- }
-
- if (cstate->opts.csv_mode)
- {
- if (string == NULL &&
- cstate->opts.force_notnull_flags[m])
- {
- /*
- * FORCE_NOT_NULL option is set and column is NULL -
- * convert it to the NULL string.
- */
- string = cstate->opts.null_print;
- }
- else if (string != NULL && cstate->opts.force_null_flags[m]
- && strcmp(string, cstate->opts.null_print) == 0)
- {
- /*
- * FORCE_NULL option is set and column matches the NULL
- * string. It must have been quoted, or otherwise the
- * string would already have been set to NULL. Convert it
- * to NULL as specified.
- */
- string = NULL;
- }
- }
-
- cstate->cur_attname = NameStr(att->attname);
- cstate->cur_attval = string;
-
- if (string != NULL)
- nulls[m] = false;
-
- if (cstate->defaults[m])
- {
- /*
- * The caller must supply econtext and have switched into the
- * per-tuple memory context in it.
- */
- Assert(econtext != NULL);
- Assert(CurrentMemoryContext == econtext->ecxt_per_tuple_memory);
-
- values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
- }
-
- /*
- * If ON_ERROR is specified with IGNORE, skip rows with soft
- * errors
- */
- else if (!InputFunctionCallSafe(&in_functions[m],
- string,
- typioparams[m],
- att->atttypmod,
- (Node *) cstate->escontext,
- &values[m]))
- {
- cstate->num_errors++;
- return true;
- }
-
- cstate->cur_attname = NULL;
- cstate->cur_attval = NULL;
- }
-
- Assert(fieldno == attr_count);
- }
- else
- {
- /* binary */
- int16 fld_count;
- ListCell *cur;
-
- cstate->cur_lineno++;
-
- if (!CopyGetInt16(cstate, &fld_count))
- {
- /* EOF detected (end of file, or protocol-level EOF) */
- return false;
- }
-
- if (fld_count == -1)
- {
- /*
- * Received EOF marker. Wait for the protocol-level EOF, and
- * complain if it doesn't come immediately. In COPY FROM STDIN,
- * this ensures that we correctly handle CopyFail, if client
- * chooses to send that now. When copying from file, we could
- * ignore the rest of the file like in text mode, but we choose to
- * be consistent with the COPY FROM STDIN case.
- */
- char dummy;
-
- if (CopyReadBinaryData(cstate, &dummy, 1) > 0)
- ereport(ERROR,
- (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
- errmsg("received copy data after EOF marker")));
- return false;
- }
-
- if (fld_count != attr_count)
- ereport(ERROR,
- (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
- errmsg("row field count is %d, expected %d",
- (int) fld_count, attr_count)));
-
- foreach(cur, cstate->attnumlist)
- {
- int attnum = lfirst_int(cur);
- int m = attnum - 1;
- Form_pg_attribute att = TupleDescAttr(tupDesc, m);
-
- cstate->cur_attname = NameStr(att->attname);
- values[m] = CopyReadBinaryAttribute(cstate,
- &in_functions[m],
- typioparams[m],
- att->atttypmod,
- &nulls[m]);
- cstate->cur_attname = NULL;
- }
- }
+ if (!cstate->routine->CopyFromOneRow(cstate, econtext, values,
+ nulls))
+ return false;
/*
* Now compute and insert any defaults available for the columns not
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index d3dc3fc854..6758a5e346 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 */
@@ -131,6 +135,340 @@ static void CopySendEndOfRow(CopyToState cstate);
static void CopySendInt32(CopyToState cstate, int32 val);
static void CopySendInt16(CopyToState cstate, int16 val);
+/*
+ * CopyToRoutine implementations.
+ */
+
+/*
+ * CopyToRoutine implementation for "text" and "csv". CopyToTextBased*() are
+ * shared by both of "text" and "csv". CopyToText*() are only for "text" and
+ * CopyToCSV*() are only for "csv".
+ *
+ * We can use the same functions for all callbacks by referring
+ * cstate->opts.csv_mode but splitting callbacks to eliminate "if
+ * (cstate->opts.csv_mode)" branches from all callbacks has performance
+ * merit when many tuples are copied. So we use separated callbacks for "text"
+ * and "csv".
+ */
+
+static void
+CopyToTextBasedSendEndOfRow(CopyToState cstate)
+{
+ switch (cstate->copy_dest)
+ {
+ case COPY_FILE:
+ /* Default line termination depends on platform */
+#ifndef WIN32
+ CopySendChar(cstate, '\n');
+#else
+ CopySendString(cstate, "\r\n");
+#endif
+ break;
+ case COPY_FRONTEND:
+ /* The FE/BE protocol uses \n as newline for all platforms */
+ CopySendChar(cstate, '\n');
+ break;
+ default:
+ break;
+ }
+ CopySendEndOfRow(cstate);
+}
+
+typedef void (*CopyAttributeOutHeaderFunction) (CopyToState cstate, char *string);
+
+/*
+ * We can use CopyAttributeOutText() directly but define this for consistency
+ * with CopyAttributeOutCSVHeader(). "static inline" will prevent performance
+ * penalty by this wrapping.
+ */
+static inline void
+CopyAttributeOutTextHeader(CopyToState cstate, char *string)
+{
+ CopyAttributeOutText(cstate, string);
+}
+
+static inline void
+CopyAttributeOutCSVHeader(CopyToState cstate, char *string)
+{
+ CopyAttributeOutCSV(cstate, string, false,
+ list_length(cstate->attnumlist) == 1);
+}
+
+/*
+ * We don't use this function as a callback directly. We define
+ * CopyToTextStart() and CopyToCSVStart() and use them instead. It's for
+ * eliminating a "if (cstate->opts.csv_mode)" branch. This callback is called
+ * only once per COPY TO. So this optimization may be meaningless but done for
+ * consistency with CopyToTextBasedOneRow().
+ *
+ * This must initialize cstate->out_functions for CopyToTextBasedOneRow().
+ */
+static inline void
+CopyToTextBasedStart(CopyToState cstate, TupleDesc tupDesc, CopyAttributeOutHeaderFunction out)
+{
+ int num_phys_attrs;
+ ListCell *cur;
+
+ num_phys_attrs = tupDesc->natts;
+ /* Get info about the columns we need to process. */
+ cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ Oid out_func_oid;
+ bool isvarlena;
+ Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
+
+ getTypeOutputInfo(attr->atttypid, &out_func_oid, &isvarlena);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
+
+ /*
+ * For non-binary copy, we need to convert null_print to file encoding,
+ * because it will be sent directly with CopySendString.
+ */
+ if (cstate->need_transcoding)
+ cstate->opts.null_print_client = pg_server_to_any(cstate->opts.null_print,
+ cstate->opts.null_print_len,
+ cstate->file_encoding);
+
+ /* if a header has been requested send the line */
+ if (cstate->opts.header_line)
+ {
+ bool hdr_delim = false;
+
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ char *colname;
+
+ if (hdr_delim)
+ CopySendChar(cstate, cstate->opts.delim[0]);
+ hdr_delim = true;
+
+ colname = NameStr(TupleDescAttr(tupDesc, attnum - 1)->attname);
+
+ out(cstate, colname);
+ }
+
+ CopyToTextBasedSendEndOfRow(cstate);
+ }
+}
+
+static void
+CopyToTextStart(CopyToState cstate, TupleDesc tupDesc)
+{
+ CopyToTextBasedStart(cstate, tupDesc, CopyAttributeOutTextHeader);
+}
+
+static void
+CopyToCSVStart(CopyToState cstate, TupleDesc tupDesc)
+{
+ CopyToTextBasedStart(cstate, tupDesc, CopyAttributeOutCSVHeader);
+}
+
+typedef void (*CopyAttributeOutValueFunction) (CopyToState cstate, char *string, int attnum);
+
+static inline void
+CopyAttributeOutTextValue(CopyToState cstate, char *string, int attnum)
+{
+ CopyAttributeOutText(cstate, string);
+}
+
+static inline void
+CopyAttributeOutCSVValue(CopyToState cstate, char *string, int attnum)
+{
+ CopyAttributeOutCSV(cstate, string,
+ cstate->opts.force_quote_flags[attnum - 1],
+ list_length(cstate->attnumlist) == 1);
+}
+
+/*
+ * We don't use this function as a callback directly. We define
+ * CopyToTextOneRow() and CopyToCSVOneRow() and use them instead. It's for
+ * eliminating a "if (cstate->opts.csv_mode)" branch. This callback is called
+ * per tuple. So this optimization will be valuable when many tuples are
+ * copied.
+ *
+ * cstate->out_functions must be initialized in CopyToTextBasedStart().
+ */
+static void
+CopyToTextBasedOneRow(CopyToState cstate,
+ TupleTableSlot *slot,
+ CopyAttributeOutValueFunction out)
+{
+ bool need_delim = false;
+ FmgrInfo *out_functions = cstate->out_functions;
+ ListCell *cur;
+
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ Datum value = slot->tts_values[attnum - 1];
+ bool isnull = slot->tts_isnull[attnum - 1];
+
+ if (need_delim)
+ CopySendChar(cstate, cstate->opts.delim[0]);
+ need_delim = true;
+
+ if (isnull)
+ {
+ CopySendString(cstate, cstate->opts.null_print_client);
+ }
+ else
+ {
+ char *string;
+
+ string = OutputFunctionCall(&out_functions[attnum - 1], value);
+ out(cstate, string, attnum);
+ }
+ }
+
+ CopyToTextBasedSendEndOfRow(cstate);
+}
+
+static void
+CopyToTextOneRow(CopyToState cstate, TupleTableSlot *slot)
+{
+ CopyToTextBasedOneRow(cstate, slot, CopyAttributeOutTextValue);
+}
+
+static void
+CopyToCSVOneRow(CopyToState cstate, TupleTableSlot *slot)
+{
+ CopyToTextBasedOneRow(cstate, slot, CopyAttributeOutCSVValue);
+}
+
+static void
+CopyToTextBasedEnd(CopyToState cstate)
+{
+}
+
+/*
+ * CopyToRoutine implementation for "binary".
+ */
+
+/*
+ * This must initialize cstate->out_functions for CopyToBinaryOneRow().
+ */
+static void
+CopyToBinaryStart(CopyToState cstate, TupleDesc tupDesc)
+{
+ int num_phys_attrs;
+ ListCell *cur;
+
+ num_phys_attrs = tupDesc->natts;
+ /* Get info about the columns we need to process. */
+ cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ Oid out_func_oid;
+ bool isvarlena;
+ Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
+
+ getTypeBinaryOutputInfo(attr->atttypid, &out_func_oid, &isvarlena);
+ fmgr_info(out_func_oid, &cstate->out_functions[attnum - 1]);
+ }
+
+ {
+ /* Generate header for a binary copy */
+ int32 tmp;
+
+ /* Signature */
+ CopySendData(cstate, BinarySignature, 11);
+ /* Flags field */
+ tmp = 0;
+ CopySendInt32(cstate, tmp);
+ /* No header extension */
+ tmp = 0;
+ CopySendInt32(cstate, tmp);
+ }
+}
+
+/*
+ * cstate->out_functions must be initialized in CopyToBinaryStart().
+ */
+static void
+CopyToBinaryOneRow(CopyToState cstate, TupleTableSlot *slot)
+{
+ FmgrInfo *out_functions = cstate->out_functions;
+ ListCell *cur;
+
+ /* Binary per-tuple header */
+ CopySendInt16(cstate, list_length(cstate->attnumlist));
+
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ Datum value = slot->tts_values[attnum - 1];
+ bool isnull = slot->tts_isnull[attnum - 1];
+
+ if (isnull)
+ {
+ CopySendInt32(cstate, -1);
+ }
+ else
+ {
+ bytea *outputbytes;
+
+ outputbytes = SendFunctionCall(&out_functions[attnum - 1], value);
+ CopySendInt32(cstate, VARSIZE(outputbytes) - VARHDRSZ);
+ CopySendData(cstate, VARDATA(outputbytes),
+ VARSIZE(outputbytes) - VARHDRSZ);
+ }
+ }
+
+ CopySendEndOfRow(cstate);
+}
+
+static void
+CopyToBinaryEnd(CopyToState cstate)
+{
+ /* Generate trailer for a binary copy */
+ CopySendInt16(cstate, -1);
+ /* Need to flush out the trailer */
+ CopySendEndOfRow(cstate);
+}
+
+/*
+ * CopyToTextBased*() are shared with "csv". CopyToText*() are only for "text".
+ */
+static const CopyToRoutine CopyToRoutineText = {
+ .CopyToStart = CopyToTextStart,
+ .CopyToOneRow = CopyToTextOneRow,
+ .CopyToEnd = CopyToTextBasedEnd,
+};
+
+/*
+ * CopyToTextBased*() are shared with "text". CopyToCSV*() are only for "csv".
+ */
+static const CopyToRoutine CopyToRoutineCSV = {
+ .CopyToStart = CopyToCSVStart,
+ .CopyToOneRow = CopyToCSVOneRow,
+ .CopyToEnd = CopyToTextBasedEnd,
+};
+
+static const CopyToRoutine CopyToRoutineBinary = {
+ .CopyToStart = CopyToBinaryStart,
+ .CopyToOneRow = CopyToBinaryOneRow,
+ .CopyToEnd = CopyToBinaryEnd,
+};
+
+/*
+ * Define the COPY TO routines to use for a format.
+ */
+static const CopyToRoutine *
+CopyToGetRoutine(CopyFormatOptions opts)
+{
+ if (opts.csv_mode)
+ return &CopyToRoutineCSV;
+ else if (opts.binary)
+ return &CopyToRoutineBinary;
+
+ /* default is text */
+ return &CopyToRoutineText;
+}
/*
* Send copy start/stop messages for frontend copies. These have changed
@@ -198,16 +536,6 @@ CopySendEndOfRow(CopyToState cstate)
switch (cstate->copy_dest)
{
case COPY_FILE:
- if (!cstate->opts.binary)
- {
- /* Default line termination depends on platform */
-#ifndef WIN32
- CopySendChar(cstate, '\n');
-#else
- CopySendString(cstate, "\r\n");
-#endif
- }
-
if (fwrite(fe_msgbuf->data, fe_msgbuf->len, 1,
cstate->copy_file) != 1 ||
ferror(cstate->copy_file))
@@ -242,10 +570,6 @@ CopySendEndOfRow(CopyToState cstate)
}
break;
case COPY_FRONTEND:
- /* The FE/BE protocol uses \n as newline for all platforms */
- if (!cstate->opts.binary)
- CopySendChar(cstate, '\n');
-
/* Dump the accumulated row as one CopyData message */
(void) pq_putmessage(PqMsg_CopyData, fe_msgbuf->data, fe_msgbuf->len);
break;
@@ -433,6 +757,9 @@ BeginCopyTo(ParseState *pstate,
/* Extract options from the statement node tree */
ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options);
+ /* Set format routine */
+ cstate->routine = CopyToGetRoutine(cstate->opts);
+
/* Process the source/target relation or query */
if (rel)
{
@@ -748,8 +1075,6 @@ DoCopyTo(CopyToState cstate)
bool pipe = (cstate->filename == NULL && cstate->data_dest_cb == NULL);
bool fe_copy = (pipe && whereToSendOutput == DestRemote);
TupleDesc tupDesc;
- int num_phys_attrs;
- ListCell *cur;
uint64 processed;
if (fe_copy)
@@ -759,32 +1084,11 @@ DoCopyTo(CopyToState cstate)
tupDesc = RelationGetDescr(cstate->rel);
else
tupDesc = cstate->queryDesc->tupDesc;
- num_phys_attrs = tupDesc->natts;
cstate->opts.null_print_client = cstate->opts.null_print; /* default */
/* We use fe_msgbuf as a per-row buffer regardless of copy_dest */
cstate->fe_msgbuf = makeStringInfo();
- /* Get info about the columns we need to process. */
- cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
- foreach(cur, cstate->attnumlist)
- {
- int attnum = lfirst_int(cur);
- Oid out_func_oid;
- bool isvarlena;
- Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
-
- if (cstate->opts.binary)
- getTypeBinaryOutputInfo(attr->atttypid,
- &out_func_oid,
- &isvarlena);
- 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
* recover palloc'd memory. This avoids any problems with leaks inside
@@ -795,57 +1099,7 @@ DoCopyTo(CopyToState cstate)
"COPY TO",
ALLOCSET_DEFAULT_SIZES);
- if (cstate->opts.binary)
- {
- /* Generate header for a binary copy */
- int32 tmp;
-
- /* Signature */
- CopySendData(cstate, BinarySignature, 11);
- /* Flags field */
- tmp = 0;
- CopySendInt32(cstate, tmp);
- /* No header extension */
- tmp = 0;
- CopySendInt32(cstate, tmp);
- }
- else
- {
- /*
- * For non-binary copy, we need to convert null_print to file
- * encoding, because it will be sent directly with CopySendString.
- */
- if (cstate->need_transcoding)
- cstate->opts.null_print_client = pg_server_to_any(cstate->opts.null_print,
- cstate->opts.null_print_len,
- cstate->file_encoding);
-
- /* if a header has been requested send the line */
- if (cstate->opts.header_line)
- {
- bool hdr_delim = false;
-
- foreach(cur, cstate->attnumlist)
- {
- int attnum = lfirst_int(cur);
- char *colname;
-
- if (hdr_delim)
- CopySendChar(cstate, cstate->opts.delim[0]);
- hdr_delim = true;
-
- colname = NameStr(TupleDescAttr(tupDesc, attnum - 1)->attname);
-
- if (cstate->opts.csv_mode)
- CopyAttributeOutCSV(cstate, colname, false,
- list_length(cstate->attnumlist) == 1);
- else
- CopyAttributeOutText(cstate, colname);
- }
-
- CopySendEndOfRow(cstate);
- }
- }
+ cstate->routine->CopyToStart(cstate, tupDesc);
if (cstate->rel)
{
@@ -884,13 +1138,7 @@ DoCopyTo(CopyToState cstate)
processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
}
- if (cstate->opts.binary)
- {
- /* Generate trailer for a binary copy */
- CopySendInt16(cstate, -1);
- /* Need to flush out the trailer */
- CopySendEndOfRow(cstate);
- }
+ cstate->routine->CopyToEnd(cstate);
MemoryContextDelete(cstate->rowcontext);
@@ -906,71 +1154,15 @@ DoCopyTo(CopyToState cstate)
static void
CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot)
{
- bool need_delim = false;
- FmgrInfo *out_functions = cstate->out_functions;
MemoryContext oldcontext;
- ListCell *cur;
- char *string;
MemoryContextReset(cstate->rowcontext);
oldcontext = MemoryContextSwitchTo(cstate->rowcontext);
- if (cstate->opts.binary)
- {
- /* 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 (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
- CopySendInt32(cstate, -1);
- }
- else
- {
- 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],
- list_length(cstate->attnumlist) == 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);
- }
- }
- }
-
- CopySendEndOfRow(cstate);
+ cstate->routine->CopyToOneRow(cstate, slot);
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 91433d439b..d02a7773e3 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -473,6 +473,7 @@ ConvertRowtypeExpr
CookedConstraint
CopyDest
CopyFormatOptions
+CopyFromRoutine
CopyFromState
CopyFromStateData
CopyHeaderChoice
@@ -482,6 +483,7 @@ CopyMultiInsertInfo
CopyOnErrorChoice
CopySource
CopyStmt
+CopyToRoutine
CopyToState
CopyToStateData
Cost
--
2.43.0
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-02 00:40 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-02 06:21 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-02 07:27 ` Junwang Zhao <[email protected]>
2024-02-02 07:47 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
0 siblings, 1 reply; 14+ messages in thread
From: Junwang Zhao @ 2024-02-02 07:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Fri, Feb 2, 2024 at 2:21 PM Michael Paquier <[email protected]> wrote:
>
> On Fri, Feb 02, 2024 at 09:40:56AM +0900, Sutou Kouhei wrote:
> > Thanks. It'll help us.
>
> I have done a review of v10, see v11 attached which is still WIP, with
> the patches for COPY TO and COPY FROM merged together. Note that I'm
> thinking to merge them into a single commit.
>
> @@ -74,11 +75,11 @@ typedef struct CopyFormatOptions
> bool convert_selectively; /* do selective binary conversion? */
> CopyOnErrorChoice on_error; /* what to do when error happened */
> List *convert_select; /* list of column names (can be NIL) */
> + const CopyToRoutine *to_routine; /* callback routines for COPY TO */
> } CopyFormatOptions;
>
> Adding the routines to the structure for the format options is in my
> opinion incorrect. The elements of this structure are first processed
> in the option deparsing path, and then we need to use the options to
> guess which routines we need. A more natural location is cstate
> itself, so as the pointer to the routines is isolated within copyto.c
I agree CopyToRoutine should be placed into CopyToStateData, but
why set it after ProcessCopyOptions, the implementation of
CopyToGetRoutine doesn't make sense if we want to support custom
format in the future.
Seems the refactor of v11 only considered performance but not
*extendable copy format*.
> and copyfrom_internal.h. My point is: the routines are an
> implementation detail that the centralized copy.c has no need to know
> about. This also led to a strange separation with
> ProcessCopyOptionFormatFrom() and ProcessCopyOptionFormatTo() to fit
> the hole in-between.
>
> The separation between cstate and the format-related fields could be
> much better, though I am not sure if it is worth doing as it
> introduces more duplication. For example, max_fields and raw_fields
> are specific to text and csv, while binary does not care much.
> Perhaps this is just useful to be for custom formats.
I think those can be placed in format specific fields by utilizing the opaque
space, but yeah, this will introduce duplication.
>
> copyapi.h needs more documentation, like what is expected for
> extension developers when using these, what are the arguments, etc. I
> have added what I had in mind for now.
>
> +typedef char *(*PostpareColumnValue) (CopyFromState cstate, char *string, int m);
>
> CopyReadAttributes and PostpareColumnValue are also callbacks specific
> to text and csv, except that they are used within the per-row
> callbacks. The same can be said about CopyAttributeOutHeaderFunction.
> It seems to me that it would be less confusing to store pointers to
> them in the routine structures, where the final picture involves not
> having multiple layers of APIs like CopyToCSVStart,
> CopyAttributeOutTextValue, etc. These *have* to be documented
> properly in copyapi.h, and this is much easier now that cstate stores
> the routine pointers. That would also make simpler function stacks.
> Note that I have not changed that in the v11 attached.
>
> This business with the extra callbacks required for csv and text is my
> main point of contention, but I'd be OK once the model of the APIs is
> more linear, with everything in Copy{From,To}State. The changes would
> be rather simple, and I'd be OK to put my hands on it. Just,
> Sutou-san, would you agree with my last point about these extra
> callbacks?
> --
> Michael
If V7 and V10 have no performance reduction, then I think V6 is also
good with performance, since most of the time goes to CopyToOneRow
and CopyFromOneRow.
I just think we should take the *extendable* into consideration at
the beginning.
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-02 00:40 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-02 06:21 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-02 07:27 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
@ 2024-02-02 07:47 ` Sutou Kouhei <[email protected]>
0 siblings, 0 replies; 14+ messages in thread
From: Sutou Kouhei @ 2024-02-02 07:47 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Hi,
In <CAEG8a3LxnBwNRPRwvmimDvOkPvYL8pB1+rhLBnxjeddFt3MeNw@mail.gmail.com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 2 Feb 2024 15:27:15 +0800,
Junwang Zhao <[email protected]> wrote:
> I agree CopyToRoutine should be placed into CopyToStateData, but
> why set it after ProcessCopyOptions, the implementation of
> CopyToGetRoutine doesn't make sense if we want to support custom
> format in the future.
>
> Seems the refactor of v11 only considered performance but not
> *extendable copy format*.
Right.
We focus on performance for now. And then we will focus on
extendability. [1]
[1] https://www.postgresql.org/message-id/flat/20240130.171511.2014195814665030502.kou%40clear-code.com#...
> If V7 and V10 have no performance reduction, then I think V6 is also
> good with performance, since most of the time goes to CopyToOneRow
> and CopyFromOneRow.
Don't worry. I'll re-submit changes in the v6 patch set
again after the current patch set that focuses on
performance is merged.
> I just think we should take the *extendable* into consideration at
> the beginning.
Introducing Copy{To,From}Routine is also valuable for
extendability. We can improve extendability later. Let's
focus on only performance for now to introduce
Copy{To,From}Routine.
Thanks,
--
kou
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
@ 2024-02-02 00:52 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2024-02-02 00:52 UTC (permalink / raw)
To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Fri, Feb 02, 2024 at 06:51:02AM +0900, Michael Paquier wrote:
> I am going to try to plug in some rusage() calls in the backend for
> the COPY paths. I hope that gives more precision about the backend
> activity. I'll post that with more numbers.
And here they are with log_statement_stats enabled to get rusage() fot
these queries:
test | user_s | system_s | elapsed_s
----------------------+----------+----------+-----------
head_to_bin_1col | 1.639761 | 0.007998 | 1.647762
v7_to_bin_1col | 1.645499 | 0.004003 | 1.649498
v10_to_bin_1col | 1.639466 | 0.004008 | 1.643488
head_to_bin_10col | 7.486369 | 0.056007 | 7.542485
v7_to_bin_10col | 7.314341 | 0.039990 | 7.354743
v10_to_bin_10col | 7.329355 | 0.052007 | 7.381408
head_to_text_1col | 1.581140 | 0.012000 | 1.593166
v7_to_text_1col | 1.615441 | 0.003992 | 1.619446
v10_to_text_1col | 1.613443 | 0.000000 | 1.613454
head_to_text_10col | 5.897014 | 0.011990 | 5.909063
v7_to_text_10col | 5.722872 | 0.016014 | 5.738979
v10_to_text_10col | 5.762286 | 0.011993 | 5.774265
head_from_bin_1col | 1.524038 | 0.020000 | 1.544046
v7_from_bin_1col | 1.551367 | 0.016015 | 1.567408
v10_from_bin_1col | 1.560087 | 0.016001 | 1.576115
head_from_bin_10col | 5.238444 | 0.139993 | 5.378595
v7_from_bin_10col | 5.170503 | 0.076021 | 5.246588
v10_from_bin_10col | 5.106496 | 0.112020 | 5.218565
head_from_text_1col | 1.664124 | 0.003998 | 1.668172
v7_from_text_1col | 1.720616 | 0.007990 | 1.728617
v10_from_text_1col | 1.683950 | 0.007990 | 1.692098
head_from_text_10col | 4.859651 | 0.015996 | 4.875747
v7_from_text_10col | 4.775975 | 0.032000 | 4.808051
v10_from_text_10col | 4.737512 | 0.028012 | 4.765522
(24 rows)
I'm looking at this table, and what I can see is still a lot of
variance in the tests with tables involving 1 attribute. However, a
second thing stands out to me here: there is a speedup with the
10-attribute case for all both COPY FROM and COPY TO, and both
formats. The data posted at [1] is showing me the same trend. In
short, let's move on with this split refactoring with the per-row
callbacks. That clearly shows benefits.
[1] https://www.postgresql.org/message-id/[email protected]
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2024-02-02 07:47 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 13:12 [PATCH 3/7] Extract code to get reason that recovery was stopped to a function. Heikki Linnakangas <[email protected]>
2021-06-21 13:12 [PATCH 3/7] Extract code to get reason that recovery was stopped to a function. Heikki Linnakangas <[email protected]>
2024-02-01 01:57 Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 03:43 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
2024-02-01 03:56 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 04:20 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
2024-02-01 03:49 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-01 15:19 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-01 21:51 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-02 00:40 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-02 06:21 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[email protected]>
2024-02-02 07:27 ` Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]>
2024-02-02 07:47 ` Re: Make COPY format extendable: Extract COPY TO format implementations Sutou Kouhei <[email protected]>
2024-02-02 00:52 ` Re: Make COPY format extendable: Extract COPY TO format implementations Michael Paquier <[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