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

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.

Regards,
Dean