Re: executor relation handling

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. 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.

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.

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. 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. 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.

Thoughts?

regards, tom lane