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