On 2018/10/04 5:16, Tom Lane wrote:
> I wrote:
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.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.
Okay.
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
Thanks,
Amit
