Re: executor relation handling

On 2018/09/30 5:04, Tom Lane wrote:
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
>> I've attached v10 which fixes this and aligns the WARNING text in
>> ExecInitRangeTable() and addRangeTableEntryForRelation().
>
> I started poking at this.

Thanks a lot for looking at this.

> I thought that it would be a good cross-check
> to apply just the "front half" of 0001 (i.e., creation and population of
> the RTE lockmode field), and then to insert checks in each of the
> "back half" places (executor, plancache, etc) that the lockmodes they
> are computing today match what is in the RTE.
>
> Those checks fell over many times in the regression tests.
>
> There seem to be at least four distinct problems:
>
> 1. You set up transformRuleStmt to insert AccessExclusiveLock into
> the "OLD" and "NEW" RTEs for a view. This is surely wrong; we do
> not want to take exclusive lock on a view just to run a query using
> the view. It should (usually, anyway) just be AccessShareLock.
>
> However, because addRangeTableEntryForRelation insists that you
> hold the requested lock type *now*, just changing the parameter
> to AccessShareLock doesn't work.
>
> I hacked around this for the moment by passing NoLock to
> addRangeTableEntryForRelation and then changing rte->lockmode
> after it returns, but man that's ugly. It makes me wonder whether
> addRangeTableEntryForRelation should be checking the lockmode at all.
>
> I'm inclined to think we should remove the check for current lockmode
> in addRangeTableEntryForRelation, and instead just assert that the
> passed lockmode must be one of AccessShareLock, RowShareLock, or
> RowExclusiveLock depending on whether the RTE is meant to represent
> a plain source table, a FOR UPDATE/SHARE source table, or a target
> table. I don't think it's that helpful to be checking that the
> caller got exactly the same lock, especially given the number of
> places where the patch had to cheat already by using NoLock.

Yeah, addRangeTableEntryForRelation should not insist on holding the
"exact" lock, but rather *at least* as strong as the passed in lock mode.
I see that the patch you posted downthread does that.

In the original patch, the lock mode used for handling the CREATE RULE
command was being conflated with the lock mode to require on NEW and OLD
RTEs that will be added to the rule's query.

> 2. The "forUpdatePushedDown" override in AcquireRewriteLocks isn't
> getting modeled in the RTE lockmode fields. In other words, if we
> have something like
>
> CREATE VIEW vv AS SELECT * FROM tab1;
> SELECT * FROM vv FOR UPDATE OF vv;
>
> the checks fall over, because the tab1 RTE in the view's rangetable
> just has AccessShareLock, but we need RowShareLock. I fixed this
> by having AcquireRewriteLocks actually modify the RTE if it is
> promoting the lock level. This is kinda grotty, but we were already
> assuming that AcquireRewriteLocks could scribble on the query tree,
> so it seems safe enough.

Thanks for fixing that.

> 3. There remain some cases where the RTE says RowExclusiveLock but
> the executor calculation indicates we only need AccessShareLock.
> AFAICT, this happens only when we have a DO ALSO rule that results
> in an added query that merely scans the target table.

I've seen something like that happen for ON CONFLICT's excluded
pseudo-relation RTE too, because the executor deems only the RTE fetched
with result relation RT index to require RowExclusiveLock.

> The RTE used
> for that purpose is just the original one, so it still has a lockmode
> suitable for a target table.
>
> We could probably hack the rewriter so that it changes the RTE lockmode
> back down to AccessShareLock in these cases.

Is the problematic RTE one coming from an action query? If so, isn't it
distinct from the original RTE and its rellockmode independently
determined based on whether the action is select or not?

I'm not sure if I understand why we'd need to change its lock mode.

> However, I'm inclined to
> think that that is something *not* to do, and that we should let the
> higher lockmode be used instead, for two reasons:
>
> (1) Asking for both AccessShareLock and RowExclusiveLock on the same
> table requires an extra trip through the shared lock manager, for little
> value that I can see.
>
> (2) If the DO ALSO rule is run before the main one, we'd be acquiring
> AccessShareLock before RowExclusiveLock, resulting in deadlock hazard
> due to lock upgrade. (I think this may be a pre-existing bug, although
> it could likely only manifest in corner cases such as where we're pulling
> a plan tree out of plancache. In most cases the first thing we'd acquire
> on a rule target table is RowExclusiveLock in the parser, before any
> rule rewriting could happen.)
>
> 4. I also notice some cases (all in FDW tests) where ExecOpenScanRelation
> is choosing AccessShareLock although the RTE has RowShareLock. These seem
> to all be cases where we're implementing FOR UPDATE/SHARE via
> ROW_MARK_COPY, so that this:
>
> ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
>
> if (erm != NULL && erm->relation != NULL)
> lockmode = NoLock;
>
> leaves lockmode as AccessShareLock because erm->relation is NULL.
> Again, I think this is probably OK, and it'd be better to use
> RowShareLock for consistency with other places that might open
> the rel. It will be a change in externally-visible behavior though.

For this and the other cases (AcquireRewriteLocks, AcquireExecutorLocks,
etc.), I wonder whether we couldn't just *not* recalculate the lock mode
based on inspecting the query tree to cross-check with rellockmode? Why
not just use rellockmode for locking? Maybe, okay to keep doing that in
debug builds though. Also, are the discrepancies like this to be
considered bugs of the existing logic?

Thanks,
Amit