Thread: Protection lost in expression eval changeover

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.  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.
        regards, tom lane

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

Andres Freund <andres@anarazel.de> writes:
> On 2017-03-28 13:52:50 -0400, Tom Lane wrote:
>> 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?

Well, in principle these errors ought to be unreachable at all; they're
only there to backstop any possible failure to notice stale plans.
I don't see a strong reason why we need to allow a dropped column to go
to null while we throw an immediate error for a change in column type.
(If there is some reason, hopefully beta testing will find it.)

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

Much rather not do that.
        regards, tom lane

On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-03-28 13:52:50 -0400, Tom Lane wrote:
> >> 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?
> 
> Well, in principle these errors ought to be unreachable at all; they're
> only there to backstop any possible failure to notice stale plans.

Not entirely - e.g. I think some of that's reachable when composite
types are involved.  But it's probably ok to error out anyway.


> I don't see a strong reason why we need to allow a dropped column to go
> to null while we throw an immediate error for a change in column type.
> (If there is some reason, hopefully beta testing will find it.)

Ok.  You're doing that?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
>> I don't see a strong reason why we need to allow a dropped column to go
>> to null while we throw an immediate error for a change in column type.
>> (If there is some reason, hopefully beta testing will find it.)

> Ok.  You're doing that?

Yeah, I'll go do that.
        regards, tom lane

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
>>> I don't see a strong reason why we need to allow a dropped column to go
>>> to null while we throw an immediate error for a change in column type.
>>> (If there is some reason, hopefully beta testing will find it.)

>> Ok.  You're doing that?

> Yeah, I'll go do that.

Or maybe not: turns out we have regression test cases that depend on this
behavior, cf the bit in create_view.sql about

-- this perhaps should be rejected, but it isn't:
alter table tt14t drop column f3;
-- f3 is still in the view but will read as nulls

You'd proposed changing that, which I agree with in principle, but
I thought your patch wasn't right.  Maybe we need to work harder on
that.

(I'm not actually sure right at the moment why this case isn't failing
in HEAD.  Maybe plpgsql is replacing the dropped column with nulls in
its result rows, so that whether the outer query special-cases them or
not doesn't affect the visible output.)

Or we could just throw an error anyway.  I'm not sure there's any
strong reason to allow such cases to work.  I think the regression
tests were only put there to ensure they don't crash, not to memorialize
behavior we consider essential.
        regards, tom lane

On 2017-03-28 15:24:28 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
> >>> I don't see a strong reason why we need to allow a dropped column to go
> >>> to null while we throw an immediate error for a change in column type.
> >>> (If there is some reason, hopefully beta testing will find it.)
> 
> >> Ok.  You're doing that?
> 
> > Yeah, I'll go do that.
> 
> Or maybe not: turns out we have regression test cases that depend on this
> behavior, cf the bit in create_view.sql about
> 
> -- this perhaps should be rejected, but it isn't:
> alter table tt14t drop column f3;
> -- f3 is still in the view but will read as nulls
> 
> You'd proposed changing that, which I agree with in principle, but
> I thought your patch wasn't right.  Maybe we need to work harder on
> that.
> 
> (I'm not actually sure right at the moment why this case isn't failing
> in HEAD.  Maybe plpgsql is replacing the dropped column with nulls in
> its result rows, so that whether the outer query special-cases them or
> not doesn't affect the visible output.)
> 
> Or we could just throw an error anyway.  I'm not sure there's any
> strong reason to allow such cases to work.  I think the regression
> tests were only put there to ensure they don't crash, not to memorialize
> behavior we consider essential.

Yea, cases like that was what I was referring to with changing behaviour
- I think it's ok to error out, as long as we do so reliably.

- Andres