From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers(at)postgreSQL(dot)org |
Subject: | Re: Protection lost in expression eval changeover |
Date: | 2017-03-28 18:13:24 |
Message-ID: | 20170328181324.e47nypa4vqg3dpk4@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Jim Nasby | 2017-03-28 18:15:06 | Missing increment of vacrelstats->pinskipped_pages |
Previous Message | Fujii Masao | 2017-03-28 18:10:38 | Re: pg_stat_wal_write statistics view |