Re: Why does ExecComputeStoredGenerated() form a heap tuple

[Peter Eisentraut]

On 2019-04-01 11:23, Peter Eisentraut wrote:
> On 2019-03-31 04:57, Andres Freund wrote:
>> while rebasing the remaining tableam patches (luckily a pretty small set
>> now!), I had a few conflicts with ExecComputeStoredGenerated(). While
>> resolving I noticed:
> 
> The core of that code was written a long time ago and perhaps hasn't
> caught up with all the refactoring going on.  I'll look through your
> proposal and update the code.

The attached patch is based on your sketch.  It's clearly better in the
long term not to rely on heap tuples here.  But in testing this change
seems to make it slightly slower, certainly not a speedup as you were
apparently hoping for.


Test setup:

create table t0 (a int, b int);
insert into t0 select generate_series (1, 10000000);  -- 10 million
\copy t0 (a) to 'test.dat';

-- for comparison, without generated column
truncate t0;
\copy t0 (a) from 'test.dat';

-- master
create table t1 (a int, b int generated always as (a * 2) stored);
truncate t1;
\copy t1 (a) from 'test.dat';

-- patched
truncate t1;
\copy t1 (a) from 'test.dat';

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

From d87b94d555c1d863a65e738a6953a1c87b0af2b2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 23 Apr 2019 10:18:39 +0200
Subject: [PATCH] Don't form heap tuple in ExecComputeStoredGenerated

---
 src/backend/executor/nodeModifyTable.c | 27 ++++++++++++--------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index 444c0c0574..216c3baa64 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,7 +292,10 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot 
*slot)
 
        values = palloc(sizeof(*values) * natts);
        nulls = palloc(sizeof(*nulls) * natts);
-       replaces = palloc0(sizeof(*replaces) * natts);
+
+       slot_getallattrs(slot);
+       memcpy(values, slot->tts_values, sizeof(*values) * natts);
+       memcpy(nulls, slot->tts_isnull, sizeof(*nulls) * natts);
 
        for (int i = 0; i < natts; i++)
        {
@@ -311,20 +312,16 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot 
*slot)
 
                        values[i] = val;
                        nulls[i] = isnull;
-                       replaces[i] = true;
                }
+               else
+                       values[i] = datumCopy(values[i], TupleDescAttr(tupdesc, 
i)->attbyval, TupleDescAttr(tupdesc, i)->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);
 }
-- 
2.21.0