Re: executor relation handling

Hi,

On 2018-10-03 16:16:11 -0400, 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. 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.

I've not really followed this thread, and just caught up to here. It
seems entirely unacceptable to not acquire locks on workers to me.
Maybe I'm missing something, but why do/did the patches in this thread
require that / introduce that? We didn't have that kind of concept
before, no? The group locking stuff should rely / require that kind of
thing, no?

Greetings,

Andres Freund