Re: Protection lost in expression eval changeover

Hi,

On 2017-03-28 13:52:50 -0400, Tom Lane wrote:
> CheckVarSlotCompatibility contains the comment
>
> * Note: we allow a reference to a dropped attribute. slot_getattr will
> * force a NULL result in such cases.
>
> While still true, that second sentence is now quite irrelevant, because we
> don't go through slot_getattr anymore.

Note it was already quite irrelevant before :( - if slot_getattr(n) was
called after slot_getattr(n+x), it'd go to the fastpath exit, and thus
not check anything. Same

> So it seems like we are missing some needed protection. I'm inclined
> to think that it'd be all right to just throw an error immediately in
> CheckVarSlotCompatibility if the target column is dropped.

Hm - so far we've pretty widely only set columns to NULL in that
case. You don't see concerns with triggering errors in cases we
previously hadn't?

I wonder if it'd not be better to add a branch to slot_deform_tuple's
main loop like

/*
* If the attribute has been dropped, set to NULL. That's important
* because various codepaths assume that the first slot->tts_nvalid
* attributes can be accessed directly via tts_values/isnull.
*/
if (unlikely(thisatt->attisdropped))
{
values[attnum] = (Datum) 0;
isnull[attnum] = true;
}

It's annoying to add a branch there, it's a pretty hot function, but it
seems like quite a worthwhile safety measure.

Greetings,

Andres Freund