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
thefirst 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
