Horton

Re: executor relation handling

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: David Rowley <david.rowley@2ndquadrant.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Jesper Pedersen <jesper.pedersen@redhat.com>, Amit Langote <amitlangote09@gmail.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2018-10-03T20:16:11Z
Lists: pgsql-hackers 
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.  To make that work, I had to make
both ExecOpenScanRelation and relation_open skip checking for lock-held
if IsParallelWorker().

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.  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.

			regards, tom lane

Commits

  1. Avoid O(N^2) cost in ExecFindRowMark().

  2. Remove some unnecessary fields from Plan trees.

  3. Restore sane locking behavior during parallel query.

  4. Remove more redundant relation locking during executor startup.

  5. In the executor, use an array of pointers to access the rangetable.

  6. Centralize executor's opening/closing of Relations for rangetable entries.

  7. Change executor to just Assert that table locks were already obtained.

  8. Change rewriter/planner/executor/plancache to depend on RTE rellockmode.

  9. Add assertions that we hold some relevant lock during relation open.

  10. Create an RTE field to record the query's lock mode for each relation.