Re: Re: [COMMITTERS] pgsql: Fix permission tests for views/tables proven empty by constraint - Mailing list pgsql-hackers

On 2013-05-08 12:30:31 -0400, Tom Lane wrote:
> Greg Stark <stark@mit.edu> writes:
> > That's kind of dismaying. ORMs have a tendency to create queries like
> > this and people may have even written such queries by hand and tested
> > them to determine that postgres was able to exclude the useless
> > relation. To have them install a security update and discover that
> > something they had previously tested no longer worked would be
> > annoying.
> 
> Turns out to be more to this than I realized before.  In an example
> such as the one I showed
> 
> select * from
>   ((select f1 as x from t1 offset 0)
>    union all
>    (select f2 as x from t2 offset 0)) ss
> where false;
> 
> where an appendrel subselect member can be proven empty on the basis
> of outer-query clauses alone, *we don't even plan that subquery*.
> The fix I had in mind for this fails to capture table references from
> such a subquery.

> We could extend setrefs.c to dig into unplanned subqueries and grab RTEs
> out of them, but that would not be a complete fix.  In particular, RTEs
> would not get made for inheritance children of parent tables mentioned
> in the query, since inheritance expansion is done by the planner.  Now,
> that wouldn't affect permissions checks because no extra permissions
> checks are done on inheritance children, but it would affect the locking
> behavior that Andres was worried about.

I first thought that is fair enough since I thought that in most if not
all cases where locking plays a user visible role the parent relation
would get locked anyway when a child relations gets locked. Turns out,
we do it only the other way round, i.e. lock child relations when we
lock a parent relation, even for most ddl in child relations.

I am not sure if its really problematic, but it seems to allow scenarios
like:

S1: BEGIN;
S1: SELECT * FROM ((SELECT * FROM parent OFFSET 0) UNION ALL (SELECT * FROM parent OFFSET 0)) f WHERE false;
-- parent is locked now, children are not
S2: BEGIN;
S2: ALTER TABLE child_1 DROP CONSTRAINT foo;
S1: SELECT * FROM parent WHERE ...
-- blocks, waiting for S1 since child_1 is locked.

This seems like somewhat confusing behaviour, although it has gone
unnoticed so far, since one normally expect that a previous lock allows
you to continue workin with a relation.

But I guess this is better fixed by making all DDL on child relations
also lock their parent relation? That seems like a good idea anyway.

I am not at all convinced that this must be fixed, but also not the
other way round. I just wanted to point this out since I am not sure
there aren't any more problematic cases.

> I think the only reliable way to make this optimization fully
> transparent would be to go ahead and plan every subquery, even when we
> know we'll discard the planning results immediately.  That seems like
> quite a lot of overkill.  I'm not really sure I buy Greg's argument
> that people might be depending on the performance benefits of not
> planning such subqueries, but I guess it's not impossible either.

I didn't understand Greg's argument as being based on performance but as
being worried about the changed locking and such from a functional
perspective. Greg?

I don't really buy the performance argument either, but I agree that we
shouldn't do all this in the back branches as the "bug" isn't really bad
and it has some potential for introducing problems.

> I'm also now pretty firmly in the camp of "let's not try at all to fix
> this in the back branches".

+1 independent of where this goes.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services