pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedProtection lost in expression eval changeover
7+ messages / 2 participants
[nested] [flat]
* Protection lost in expression eval changeover
@ 2017-03-28 17:52 Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2017-03-28 17:52 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Protection lost in expression eval changeover
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
@ 2017-03-28 18:13 ` Andres Freund <andres@anarazel.de>
2017-03-28 18:43 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2017-03-28 18:13 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Protection lost in expression eval changeover
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
@ 2017-03-28 18:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:51 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2017-03-28 18:43 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Protection lost in expression eval changeover
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
2017-03-28 18:43 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
@ 2017-03-28 18:51 ` Andres Freund <andres@anarazel.de>
2017-03-28 18:54 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2017-03-28 18:51 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Protection lost in expression eval changeover
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
2017-03-28 18:43 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:51 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
@ 2017-03-28 18:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 19:24 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2017-03-28 18:54 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Protection lost in expression eval changeover
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
2017-03-28 18:43 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:51 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
2017-03-28 18:54 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
@ 2017-03-28 19:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 19:28 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2017-03-28 19:24 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Protection lost in expression eval changeover
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
2017-03-28 18:43 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:51 ` Re: Protection lost in expression eval changeover Andres Freund <andres@anarazel.de>
2017-03-28 18:54 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 19:24 ` Re: Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
@ 2017-03-28 19:28 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2017-03-28 19:28 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2017-03-28 19:28 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28 17:52 Protection lost in expression eval changeover Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:13 ` Andres Freund <andres@anarazel.de>
2017-03-28 18:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 18:51 ` Andres Freund <andres@anarazel.de>
2017-03-28 18:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 19:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-03-28 19:28 ` Andres Freund <andres@anarazel.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox