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