Re: executor relation handling

On 2018/09/27 18:15, David Rowley wrote:
> I've just completed a review of the v5 patch set. I ended up just
> making the changes myself since Amit mentioned he was on leave for a
> few weeks.
>
> Summary of changes:
>
> 1. Changed the way we verify the lock already exists with debug
> builds. I reverted some incorrect code added to LockRelationOid that
> seems to have gotten broken after being rebased on f868a8143a9. I've
> just added some functions that verify the lock is in the
> LockMethodLocalHash hashtable.

Thanks. I guess I wasn't terribly happy with my job of rebasing on top of
f868a8143a9, because that commit had made the result of
LockAcquireExtended a bit ambiguous for me.

I like the new CheckRelationLockedByUs() and LocalLockExists() functions
that you added to lock manager.

> 2. Fixed some incorrect lock types being passed into
> addRangeTableEntryForRelation()
>
> 3. Added code in addRangeTableEntryForRelation to verify we actually
> hold the lock that the parameter claims we do. (This found all the
> errors I fixed in #2)

I see that I'd falsely copy-pasted AccessShareLock in transformRuleStmt,
which you've corrected to AccessExclusiveLock. I was not sure about other
cases where you've replaced NoLock by something else, because they won't
be checked or asserted, but perhaps that's fine.

> 4. Updated various comments outdated by the patches
> 5. Updated executor README's mention that we close relations when
> calling the end node function. This is now handled at the end of
> execution.

Thank you. I see that you also fixed some useless code that I had left
lying around as result of code movement such as the following dead code:

@@ -2711,9 +2711,7 @@ ExecEndModifyTable(ModifyTableState *node)
{
int i;

- /*
- * close the result relation(s) if any, but hold locks until xact commit.
- */
+ /* Perform cleanup. */
for (i = 0; i < node->mt_nplans; i++)
{
ResultRelInfo *resultRelInfo = node->resultRelInfo + i;
@@ -2728,7 +2726,6 @@ ExecEndModifyTable(ModifyTableState *node)
/* Close indices and then the relation itself */
ExecCloseIndices(resultRelInfo);
heap_close(resultRelInfo->ri_RelationDesc, NoLock);
- resultRelInfo++;
}

> 6. Renamed nominalRelation to targetRelation. I think this fits
> better since we're overloading the variable.

That makes sense to me.

> 7. Use LOCKMODE instead of int in some places.
> 8. Changed warning about relation not locked to WARNING instead of NOTICE.

Oops, think I'd forgotten to do that myself.

> 9. Renamed get_unpruned_rowmarks() to get_nondummy_rowmarks(). Pruning
> makes me think of partition pruning but the function checks for dummy
> rels. These could be dummy for reasons other than partition pruning.

Makes sense too.

> I've attached a diff showing the changes I made along with the full
> patches which I tagged as v6.

Thanks a lot for working on that. I've made minor tweaks, which find in
the attached updated patches (a .diff file containing changes from v6 to
v7 is also attached).

Regards,
Amit