Re: [v9.2] Fix leaky-view problem, part 1

I took a look at this patch. It's incredibly simple, which is great, and it
seems to achieve its goal.

Suppose your query references two views owned by different roles. The quals
of those views will have the same depth. Is there a way for information to
leak from one view owner to another due to that?

I like how you've assumed that the table owner trusts the operator class
functions of indexes on his table to not leak information. That handily
catches some basic and important qual pushdowns that would otherwise be lost.

This makes assumptions, at a distance, about the possible scan types and how
quals can be fitted to them. Specifically, this patch achieves its goals
because any indexable qual is trustworthy, and any non-indexable qual cannot
be pushed down further than the view's own quals. That seems to be true with
current plan types, but it feels fragile. I don't have a concrete idea for
improvement, though. Robert suggested another approach in
http://archives.postgresql.org/message-id/AANLkTimbN_6tYxReh5Rc7pmizT-VJB3xgp8CuHO0OAHC@mail.gmail.com
; might that approach avoid this hazard?

The part 2 patch in this series implements the planner restriction more likely
to yield performance regressions, so it introduces a mechanism for identifying
when to apply the restriction. This patch, however, applies its restriction
unconditionally. Since we will inevitably have a such mechanism before you
are done sealing the leaks in our view implementation, the restriction in this
patch should also use that mechanism. Therefore, I think we should review and
apply part 2 first, then update this patch to use its conditional behavior.

A few minor questions/comments on the implementation:

On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
> --- a/src/backend/optimizer/plan/planner.c
> +++ b/src/backend/optimizer/plan/planner.c

> + else if (IsA(node, Query))
> + {
> + depth += 2;

Why two?

> --- a/src/test/regress/expected/select_views.out
> +++ b/src/test/regress/expected/select_views.out

> +EXPLAIN SELECT * FROM your_credit WHERE f_leak(number,expired);
> + QUERY PLAN
> +--------------------------------------------------------------------------
> + Seq Scan on credit_cards (cost=0.00..181.20 rows=1 width=96)

Use "EXPLAIN (COSTS OFF)" in regression tests. We do not put much effort into
the stability of exact cost values, and they do not matter for the purpose of
this test.

> --- a/src/test/regress/sql/select_views.sql
> +++ b/src/test/regress/sql/select_views.sql
> @@ -8,3 +8,46 @@ SELECT * FROM street;
> SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>
> SELECT * FROM toyemp WHERE name = 'sharon';
> +
> +--
> +-- Test for leaky-view problem
> +--
> +
> +-- setups
> +SET client_min_messages TO 'warning';
> +
> +DROP ROLE IF EXISTS alice;
> +DROP FUNCTION IF EXISTS f_leak(text);
> +DROP TABLE IF EXISTS credit_cards;
> +
> +RESET client_min_messages;

No need for this. The regression tests always start on a clean database.

> +
> +CREATE USER alice;
> +CREATE FUNCTION f_leak(text, text)
> + RETURNS bool LANGUAGE 'plpgsql'
> + AS 'begin raise notice ''% => %'', $1, $2; return true; end';

I ran this test case on master, and it did not reproduce the problem.
However, adding "COST 0.1" to this CREATE FUNCTION did yield the expected
problem behavior. I suggest adding that.

Thanks,
nm