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
Message-ID: 987ce765-7dac-fb14-815b-6b4e0d7c7c5a@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
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 */
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

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

Responses

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