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.
