On 2019-04-24 00:26, David Rowley wrote: > I didn't do the exact same test, but if I use COPY instead of \copy, > then for me patched is faster.
OK, confirmed that way, too. > For the patch, I wonder if you need this line: > > + memcpy(values, slot->tts_values, sizeof(*values) * natts); > > If you got rid of that and changed the datumCopy to use > slot->tts_values[i] instead. done > Maybe it's also worth getting rid of the first memcpy for the null > array and just assign the element in the else clause. Tried that, seems to be slower. So I left it as is. > It might also be cleaner to assign TupleDescAttr(tupdesc, i) to a > variable instead of using the macro 3 times. It'd make that datumCopy > line shorter too. Also done. Updated patch attached. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 8617ec7576c88081f85e4498ab558e7c5aa909ec Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Wed, 15 May 2019 19:37:52 +0200 Subject: [PATCH v2] Convert ExecComputeStoredGenerated to use tuple slots This code was still using the old style of forming a heap tuple rather than using tuple slots. This would be less efficient if a non-heap access method was used. And using tuple slots is actually quite a bit faster when using heap as well. Also add some test cases for generated columns with null values and with varlena values. This lack of coverage was discovered while working on this patch. Discussion: https://www.postgresql.org/message-id/flat/20190331025744.ugbsyks7czfcoksd%40alap3.anarazel.de --- src/backend/executor/nodeModifyTable.c | 33 +++++++++++++------------ src/test/regress/expected/generated.out | 31 ++++++++++++++++++++--- src/test/regress/sql/generated.sql | 10 ++++++-- 3 files changed, 52 insertions(+), 22 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index d545bbce8a..d3a0dece5a 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -53,6 +53,7 @@ #include "storage/bufmgr.h" #include "storage/lmgr.h" #include "utils/builtins.h" +#include "utils/datum.h" #include "utils/memutils.h" #include "utils/rel.h" @@ -254,9 +255,6 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot) MemoryContext oldContext; Datum *values; bool *nulls; - bool *replaces; - HeapTuple oldtuple, newtuple; - bool should_free; Assert(tupdesc->constr && tupdesc->constr->has_generated_stored); @@ -294,11 +292,15 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot) values = palloc(sizeof(*values) * natts); nulls = palloc(sizeof(*nulls) * natts); - replaces = palloc0(sizeof(*replaces) * natts); + + slot_getallattrs(slot); + memcpy(nulls, slot->tts_isnull, sizeof(*nulls) * natts); for (int i = 0; i < natts; i++) { - if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED) + Form_pg_attribute attr = TupleDescAttr(tupdesc, i); + + if (attr->attgenerated == ATTRIBUTE_GENERATED_STORED) { ExprContext *econtext; Datum val; @@ -311,20 +313,19 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot) values[i] = val; nulls[i] = isnull; - replaces[i] = true; + } + else + { + if (!nulls[i]) + values[i] = datumCopy(slot->tts_values[i], attr->attbyval, attr->attlen); } } - oldtuple = ExecFetchSlotHeapTuple(slot, true, &should_free); - newtuple = heap_modify_tuple(oldtuple, tupdesc, values, nulls, replaces); - /* - * The tuple will be freed by way of the memory context - the slot might - * only be cleared after the context is reset, and we'd thus potentially - * double free. - */ - ExecForceStoreHeapTuple(newtuple, slot, false); - if (should_free) - heap_freetuple(oldtuple); + ExecClearTuple(slot); + memcpy(slot->tts_values, values, sizeof(*values) * natts); + memcpy(slot->tts_isnull, nulls, sizeof(*nulls) * natts); + ExecStoreVirtualTuple(slot); + ExecMaterializeSlot(slot); MemoryContextSwitchTo(oldContext); } diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out index d4ed3f7ae1..f62c93f468 100644 --- a/src/test/regress/expected/generated.out +++ b/src/test/regress/expected/generated.out @@ -226,15 +226,16 @@ NOTICE: merging multiple inherited definitions of column "b" ERROR: inherited column "b" has a generation conflict DROP TABLE gtesty; -- test stored update -CREATE TABLE gtest3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 3) STORED); -INSERT INTO gtest3 (a) VALUES (1), (2), (3); +CREATE TABLE gtest3 (a int, b int GENERATED ALWAYS AS (a * 3) STORED); +INSERT INTO gtest3 (a) VALUES (1), (2), (3), (NULL); SELECT * FROM gtest3 ORDER BY a; a | b ---+--- 1 | 3 2 | 6 3 | 9 -(3 rows) + | +(4 rows) UPDATE gtest3 SET a = 22 WHERE a = 2; SELECT * FROM gtest3 ORDER BY a; @@ -243,7 +244,29 @@ SELECT * FROM gtest3 ORDER BY a; 1 | 3 3 | 9 22 | 66 -(3 rows) + | +(4 rows) + +CREATE TABLE gtest3a (a text, b text GENERATED ALWAYS AS (a || '+' || a) STORED); +INSERT INTO gtest3a (a) VALUES ('a'), ('b'), ('c'), (NULL); +SELECT * FROM gtest3a ORDER BY a; + a | b +---+----- + a | a+a + b | b+b + c | c+c + | +(4 rows) + +UPDATE gtest3a SET a = 'bb' WHERE a = 'b'; +SELECT * FROM gtest3a ORDER BY a; + a | b +----+------- + a | a+a + bb | bb+bb + c | c+c + | +(4 rows) -- COPY TRUNCATE gtest1; diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql index da11b8c9b8..6a56ae260f 100644 --- a/src/test/regress/sql/generated.sql +++ b/src/test/regress/sql/generated.sql @@ -95,12 +95,18 @@ CREATE TABLE gtest1_2 () INHERITS (gtest1, gtesty); -- error DROP TABLE gtesty; -- test stored update -CREATE TABLE gtest3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 3) STORED); -INSERT INTO gtest3 (a) VALUES (1), (2), (3); +CREATE TABLE gtest3 (a int, b int GENERATED ALWAYS AS (a * 3) STORED); +INSERT INTO gtest3 (a) VALUES (1), (2), (3), (NULL); SELECT * FROM gtest3 ORDER BY a; UPDATE gtest3 SET a = 22 WHERE a = 2; SELECT * FROM gtest3 ORDER BY a; +CREATE TABLE gtest3a (a text, b text GENERATED ALWAYS AS (a || '+' || a) STORED); +INSERT INTO gtest3a (a) VALUES ('a'), ('b'), ('c'), (NULL); +SELECT * FROM gtest3a ORDER BY a; +UPDATE gtest3a SET a = 'bb' WHERE a = 'b'; +SELECT * FROM gtest3a ORDER BY a; + -- COPY TRUNCATE gtest1; INSERT INTO gtest1 (a) VALUES (1), (2); base-commit: aa4b8c61d2cd57b53be03defb04d59b232a0e150 -- 2.21.0
