Re: WIP patch (v2) for updatable security barrier views

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 10 March 2014 03:36, Craig Ringer <craig@2ndquadrant.com> wrote:
>> I've found an issue with updatable security barrier views. Locking is
>> being pushed down into the subquery. Locking is thus applied before
>> user-supplied quals are, so we potentially lock too many rows.

> That has nothing to do with *updatable* security barrier views,
> because that's not an update. In fact you get that exact same plan on
> HEAD, without the updatable security barrier views patch.

I got around to looking at this.  The proximate reason for the two
LockRows nodes is:

(1) The parser and rewriter intentionally insert RowMarkClauses into
both the outer query and the subquery when a FOR UPDATE/SHARE applies
to a subquery-in-FROM.  The comment in transformLockingClause explains
why:
                    * FOR UPDATE/SHARE of subquery is propagated to all of                    * subquery's rels, too.
Wecould do this later (based on                    * the marking of the subquery RTE) but it is convenient
     * to have local knowledge in each query level about which                    * rels need to be opened with
RowShareLock.

that is, if we didn't push down the RowMarkClause, processing of the
subquery would be at risk of opening relations with too weak a lock.
In the example case, this pushdown is actually done by the rewriter's
markQueryForLocking function, but it's just emulating what the parser
would have done if the view query had been written in-line.

(2) The planner doesn't consider this, and generates a LockRows plan node
for each level of the query, since both of them have rowMarks.

However, I'm not sure this is a bug, or at least it's not the same bug you
think it is.  The lower LockRows node is doing a ROW_MARK_EXCLUSIVE mark
on the physical table rows, while the upper one is doing a ROW_MARK_COPY
on the virtual rows emitted by the subquery.  *These are not the same
thing*.  A ROW_MARK_COPY isn't a lock of any sort; it's just there to
allow EvalPlanQual to see the row that was emitted from the subquery,
in case a recheck has to be done during a Read Committed UPDATE/DELETE.
Since this is a pure SELECT, the upper "locking" action is useless, and
arguably the planner ought to be smart enough not to emit it.  But it's
just wasting some cycles, it's not changing any semantics.

So if you wanted user-supplied quals to limit which rows get locked
physically, they would need to be applied before the lower LockRows node.

To my mind, it's not immediately apparent that that is a reasonable
expectation.  The entire *point* of a security_barrier view is that
unsafe quals don't get pushed into it, so why would you expect that
those quals get applied early enough to limit locking?
        regards, tom lane