Re: [v9.2] Fix Leaky View Problem

On Mon, Sep 26, 2011 at 3:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> Sorry, are you saying the current (in other words, rte->security_barrier
>> stores the state of reloption) approach is not a good idea?
>
> Yes.  I think the same as Robert: the way to handle this is to store it
> in RelOptInfo for the duration of planning, and pull it from the
> catalogs during planner startup (cf plancat.c).

Having looked at this more, I'm starting to believe KaiGai has this
part right after all. The trouble is that the rewriter does this:

/*
* Now, plug the view query in as a subselect, replacing the relation's
* original RTE.
*/
rte = rt_fetch(rt_index, parsetree->rtable);
rte->rtekind = RTE_SUBQUERY;
rte->relid = InvalidOid;
rte->subquery = rule_action;
rte->inh = false; /* must not be set for a subquery */

In other words, by the time the planner comes along and tries to
decide whether or not it should flatten this subquery, the view has
already been rewritten into a subquery - and that subquery is in most
respects indistinguishable from a subquery that the user wrote
directly. There is one difference: the permission check that would
have been done against the view gets attached to the OLD entry in the
subquery's range table. It would probably be possible to make this
work by having the code paths that need to know whether or not a given
subquery originated from a security-barrier-enabled view do that same
trick: peek down into the OLD entry in the subquery rangetable,
extract the view OID from there, and go check its reloptions. But
that seems awfully complicated and error-prone, hence my feeling that
just flagging the subquery explicitly is probably a better approach.

One other possibility that comes to mind is that, instead of adding
"bool security_view" to the RTE, we could instead add a new RTEKind,
something like RTE_SECURITY_VIEW. That would mean going through and
finding all the places that refer to RTE_SUBQUERY and adjusting them
to handle RTE_SECURITY_VIEW in either the same way or differently as
may be appropriate. The possible advantage of this approach is that
it doesn't bloat the RTE structure (and stored rules that use it) with
an additional attribute that (I think) will always be false - because
security_barrier can only be set on a subquery RTE after rewriting has
happened, and stored rules are haven't been rewritten yet. It might
also force people to think a bit more carefully about how security
views should be handled during future code changes, which could also
be viewed as a plus.

I'm attaching my current version of KaiGai's patch (with substantial
cleanup of the comments and documentation, and some other changes) for
reference.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company