Re: Virtual generated columns

On 12.11.24 17:50, Alvaro Herrera wrote:
>> On 12.11.24 09:49, jian he wrote:
>>>> On Wed, Nov 6, 2024 at 12:17 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> 
>>> check_modified_virtual_generated, we can replace fastgetattr to
>>> heap_attisnull? like:
>>>               // bool        isnull;
>>>               // fastgetattr(tuple, i + 1, tupdesc, &isnull);
>>>               // if (!isnull)
>>>               //     ereport(ERROR,
>>>               //             (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
>>>               //              errmsg("trigger modified virtual generated
>>> column value")));
>>>               if (!heap_attisnull(tuple, i+1, tupdesc))
>>>                   ereport(ERROR,
>>>                           (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
>>>                            errmsg("trigger modified virtual generated
>>> column value")));
>>
>> I don't know.  fastgetattr() is supposed to be "fast". ;-)  It's all inline
>> functions, so maybe that is actually correct.  I don't have a strong opinion
>> either way.
> 
> I think Jian is right: if you're only interested in the isnull bit, then
> heap_attisnull is more appropriate, because it doesn't have to decode
> ("deform") the tuple before giving you the answer; it knows the answer
> by checking just the nulls bitmap.  With fastgetattr you still fetch the
> value from the data bytes, even though your function doesn't care about
> it.  That's probably even measurable for wide tuples if the generated
> attrs are at the end, which sounds common.

Ok, I have fixed that in v10.

> Personally I dislike using 0-based loops for attribute numbers, which
> are 1-based.  For peace of mind, I'd write this as
> 
>     for (AttrNumber i = 1; i <= tupdesc->natts; i++)
>     {
>         if (TupleDescAttr(tupdesc, i - 1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
>         {
>             bool        isnull;
> 
>             fastgetattr(tuple, i, tupdesc, &isnull); // heap_attisnull here actually
> 
> I'm kind of annoyed that TupleDescAttr() was made to refer to array
> indexes rather than attribute numbers, but by the time I realized it had
> happened, it was too late.

Yes, this is unfortunately a constant source of confusion.