|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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
> 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
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)
- * 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 */
> 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).
|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|