Re: executor relation handling

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
Cc: Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: executor relation handling
Date: 2018-09-28 08:00:00
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 */
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).


Attachment Content-Type Size
v6_to_v7_changes.diff text/plain 2.8 KB
v7-0001-Don-t-lock-range-table-relations-in-the-executor.patch text/plain 44.0 KB
v7-0002-Remove-useless-fields-from-planner-nodes.patch text/plain 43.4 KB
v7-0003-Prune-PlanRowMark-of-relations-that-are-pruned-fr.patch text/plain 2.4 KB
v7-0004-Revise-executor-range-table-relation-opening-clos.patch text/plain 39.7 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2018-09-28 08:21:02 Re: executor relation handling
Previous Message Peter Eisentraut 2018-09-28 07:35:48 Re: transction_timestamp() inside of procedures