Re: executor relation handling

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

On 2018/10/04 5:16, Tom Lane wrote:
> I wrote:
>> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>>> Should this check that we're not in a parallel worker process?
>> Hmm. I've not seen any failures in the parallel parts of the regular
>> regression tests, but maybe I'd better do a force_parallel_mode
>> run before committing.
>> In general, I'm not on board with the idea that parallel workers don't
>> need to get their own locks, so I don't really want to exclude parallel
>> workers from this check. But if it's not safe for that today, fixing it
>> is beyond the scope of this particular patch.
> So the place where that came out in the wash is the commit I just made
> (9a3cebeaa) to change the executor from taking table locks to asserting
> that somebody else took them already.

Thanks for getting that done.

> To make that work, I had to make
> both ExecOpenScanRelation and relation_open skip checking for lock-held
> if IsParallelWorker().

Yeah, I had to do that to when rebasing the remaining patches.

> This makes me entirely uncomfortable with the idea that parallel workers
> can be allowed to not take any locks of their own. There is no basis
> for arguing that we have field proof that that's safe, because *up to
> now, parallel workers in fact did take their own locks*. And it seems
> unsafe on its face, because there's nothing that really guarantees that
> the parent process won't go away while children are still running.
> (elog(FATAL) seems like a counterexample, for instance.)
> I think that we ought to adjust parallel query to insist that children
> do take locks, and then revert the IsParallelWorker() exceptions I made
> here.

Maybe I'm missing something here, but isn't the necessary adjustment just
that the relations are opened with locks if inside a parallel worker?

> I plan to leave that point in abeyance till we've got the rest
> of these changes in place, though. The easiest way to do it will
> doubtless change once we've centralized the executor's table-opening
> logic, so trying to code it right now seems like a waste of effort.


I've rebased the remaining patches. I broke down one of the patches into
2 and re-ordered the patches as follows:

0001: introduces a function that opens range table relations and maintains
them in an array indexes by RT index

0002: introduces a new field in EState that's an array of RangeTblEntry
pointers and revises macros used in the executor that access RTEs to
return them from the array (David Rowley co-authored this one)

0003: moves result relation and ExecRowMark initialization out of InitPlan
and into ExecInit* routines of respective nodes

0004: removes useless fields from certain planner nodes whose only purpose
has been to assist the executor lock relations in proper order

0005: teaches planner to remove PlanRowMarks corresponding to dummy relations


Attachment Content-Type Size
v12-0001-Revise-executor-range-table-relation-locking-and.patch text/plain 26.7 KB
v12-0002-Introduce-an-array-of-RangeTblEntry-pointers-in-.patch text/plain 13.6 KB
v12-0003-Move-result-rel-and-ExecRowMark-initilization-to.patch text/plain 13.9 KB
v12-0004-Remove-useless-fields-from-planner-nodes.patch text/plain 44.3 KB
v12-0005-Prune-PlanRowMark-of-relations-that-are-pruned-f.patch text/plain 2.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2018-10-04 07:49:31 Re: partition tree inspection functions
Previous Message Adrien Nayrat 2018-10-04 06:54:57 Re: Skylake-S warning