Re: Virtual generated columns

On 2024-Nov-12, Peter Eisentraut wrote:

> On 12.11.24 09:49, jian he wrote:
> > > On Wed, Nov 6, 2024 at 12:17 AM Peter Eisentraut <peter(at)eisentraut(dot)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.

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.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)