From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org, Peter Eisentraut <peter_e(at)gmx(dot)net> |
Subject: | Re: Why does ExecComputeStoredGenerated() form a heap tuple |
Date: | 2019-03-31 03:00:33 |
Message-ID: | 20190331030033.3umioadrukqiiee4@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2019-03-30 19:57:44 -0700, 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:
>
> oldtuple = ExecFetchSlotHeapTuple(slot, true, &should_free);
> newtuple = heap_modify_tuple(oldtuple, tupdesc, values, nulls, replaces);
> ExecForceStoreHeapTuple(newtuple, slot);
> if (should_free)
> heap_freetuple(oldtuple);
>
> MemoryContextSwitchTo(oldContext);
>
> First off, I'm not convinced this is correct:
>
> ISTM you'd need at least an ExecMaterializeSlot() before the
> MemoryContextSwitchTo() in ExecComputeStoredGenerated().
>
> But what actually brought me to reply was that it seems like it'll cause
> unnecessary slowdowns for !heap AMs. First, it'll form a heaptuple if
> the slot isn't in that form, and then it'll cause a conversion by
> storing a heap tuple even if the target doesn't use heap representation.
>
> ISTM the above would be much more efficiently - even more efficient if
> only heap is used - implemented as something roughly akin to:
>
> slot_getallattrs(slot);
> memcpy(values, slot->tts_values, ...);
> memcpy(nulls, slot->tts_isnull, ...);
>
> for (int i = 0; i < natts; i++)
> {
> if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED)
> {
> values[i] = ...
> }
> else
> values[i] = datumCopy(...);
> }
>
> ExecClearTuple(slot);
> memcpy(slot->tts_values, values, ...);
> memcpy(slot->tts_isnull, nulls, ...);
> ExecStoreVirtualTuple(slot);
> ExecMaterializeSlot(slot);
>
> that's not perfect, but more efficient than your version...
Also, have you actually benchmarked this code? ISTM that adding a
stored generated column would cause quite noticable slowdowns in the
COPY path based on this code.
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Erik Rijkers | 2019-03-31 03:49:04 | Re: [HACKERS] generated columns |
Previous Message | Andres Freund | 2019-03-31 02:57:44 | Why does ExecComputeStoredGenerated() form a heap tuple |