On 28 November 2016 at 23:55, Stephen Frost <sfrost@snowman.net> wrote:
>> diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
> [...]
>> --- 2104,2114 ----
>>
>> EXPLAIN (VERBOSE, COSTS OFF)
>> UPDATE v1 SET a=100 WHERE snoop(a) AND leakproof(a) AND a = 3;
>> ! QUERY PLAN
>> ! --------------------------
>> ! Result
>> ! One-Time Filter: false
>> ! (2 rows)
>
> Perhaps Dean recalls something more specific, but reviewing this test
> and the others around it, I believe it was simply to see what happens in
> a case which doesn't pass the security barrier view constraint and to
> cover the same cases with the UPDATE as were in the SELECTs above it. I
> don't see it being an issue that it now results in a one-time filter:
> false result.
>
Hmm. I've not read any of the new code yet, but the fact that this
test now reduces to a one-time filter makes it effectively useless as
a test of qual evaluation order because it has deduced that it doesn't
need to evaluate them. I would suggest replacing the qual with
something that can't be reduced, perhaps "2*a = 6".
In addition, I think that the tests on this view are probably no
longer adequate for the purpose of validating that the qual evaluation
order is safe. With the old implementation, the subquery scans in the
plans made it pretty clear that it was safe, and likely to remain safe
with variants of those queries, but that's not so obvious with the new
plans. Maybe some additional quals could be added to the view
definition, perhaps based on the other view columns, to verify that
the outer leaky qual always gets evaluated after the security barrier
quals, regardless of cost. Or perhaps that's something that's better
proved with an all-new set of tests, but it does seem to me that the
new implementation has a higher risk (or at least introduces different
risks) of unsafe evaluation orders that warrant some additional
testing.
Regards,
Dean
