Re: missing locking in at least INSERT INTO view WITH CHECK

On 2013-10-24 20:58:55 +0100, Dean Rasheed wrote:
> On 24 October 2013 18:28, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote:
> >> On 23 October 2013 21:08, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote:
> >> >> Hmm, my first thought is that rewriteTargetView() should be calling
> >> >> AcquireRewriteLocks() on viewquery, before doing too much with it.
> >> >> There may be sub-queries in viewquery's quals (and also now in its
> >> >> targetlist) and I don't think the relations referred to by those
> >> >> sub-queries are getting locked.
> >> >
> >> > Well, that wouldn't follow the currently documented rule ontop
> >> > of QueryRewrite:
> >> > * NOTE: the parsetree must either have come straight from the parser,
> >> > * or have been scanned by AcquireRewriteLocks to acquire suitable locks.
> >> >
> >> > It might still be the right thing to do, but it seems suspicious that
> >> > the rules need to be tweaked like that.
> >> >
> >>
> >> Well it matches what already happens in other places in the rewriter
> >> --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely
> >> because the rule action's query hasn't come from the parser that it
> >> needs to be processed in this way.
> >
> > I really don't know that are of code that well, fortunately I never had
> > to wade around it much so far...
> >
> > Reading your explanation and a bit of the code your plan sound sane. Are
> > you going to propose a patch?
> >
>
> I thought about making rewriteTargetView() call AcquireRewriteLocks(),
> but on closer inspection I think that is probably over the top. 90% of
> the code in AcquireRewriteLocks() is to process the query's RTEs, but
> rewriteTargetView() is already locking the single RTE in viewquery
> anyway. Also AcquireRewriteLocks() would acquire the wrong kind of
> lock on that RTE, since viewquery is a SELECT, but we want an
> exclusive lock because the RTE is about to become the outer query's
> result relation. AcquireRewriteLocks() also processes CTEs, but we
> know that we won't have any of those in rewriteTargetView(). So I
> think the only thing missing from rewriteTargetView() is to lock any
> relations from any sublink subqueries in viewquery using
> acquireLocksOnSubLinks() -- patch attached.

This is still an open problem :(

> diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
> new file mode 100644
> index c52a374..e6a9e7b
> *** a/src/backend/rewrite/rewriteHandler.c
> --- b/src/backend/rewrite/rewriteHandler.c
> *************** rewriteTargetView(Query *parsetree, Rela
> *** 2589,2594 ****
> --- 2589,2604 ----
> heap_close(base_rel, NoLock);
>
> /*
> + * If the view query contains any sublink subqueries, we should also
> + * acquire locks on any relations they refer to. We know that there won't
> + * be any subqueries in the range table or CTEs, so we can skip those, as
> + * in AcquireRewriteLocks.
> + */
> + if (viewquery->hasSubLinks)
> + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL,
> + QTW_IGNORE_RC_SUBQUERIES);
> +
> + /*
> * Create a new target RTE describing the base relation, and add it to the
> * outer query's rangetable. (What's happening in the next few steps is
> * very much like what the planner would do to "pull up" the view into the

These days this seems to require a context parameter being passed
down. Other than that this patch still fixes the problem.

Greetings,

Andres Freund