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