Re: executor relation handling

On 2018/10/07 3:59, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> On 2018/10/05 5:59, Tom Lane wrote:
>>> So I'm inclined to just omit 0003. AFAICS this would only mean that
>>> we couldn't drop the global PlanRowMarks list from PlannedStmt, which
>>> does not bother me much.
>
>> To be honest, I too had begun to fail to see the point of this patch since
>> yesterday. In fact, getting this one to pass make check-world took a bit
>> of head-scratching due to its interaction with EvalPlanQuals EState
>> building, so I was almost to drop it from the series. But I felt that it
>> might still be a good idea to get rid of what was described as special
>> case code. Reading your argument against it based on how it messes up
>> some of the assumptions regarding ExecRowMark design, I'm willing to let
>> go of this one as more or less a cosmetic improvement.
>
> OK. We do need to fix the comments in InitPlan that talk about acquiring
> locks and how that makes us need to do things in a particular order, but
> I'll go take care of that.

Thanks for doing that.

>> So, that leaves us with only the patch that revises the plan node fields
>> in light of having relieved the executor of doing any locking of its own
>> in *some* cases. That patch's premise is that we don't need the fields
>> that the patch removes because they're only referenced for the locking
>> purposes. But, if those plan nodes support parallel execution and hence
>> will be passed to parallel workers for execution who will need to lock the
>> tables contained in the plan nodes, then they better contain all the
>> information needed for locking *every* affected tables, so we had better
>> not removed it.
>
> No, I think this is unduly pessimistic. We need to make sure that a
> parallel worker has lock on tables it's actually touching, but I don't
> see why that should imply a requirement to hold lock on parent tables
> it never touches.
>
> The reasons why we need locks on tables not physically accessed by the
> query are (a) to ensure that we've blocked, or received sinval messages
> for, any DDL related to views or partition parent tables, in case that
> would invalidate the plan; (b) to allow firing triggers safely, in
> the case of partition parent tables. Neither of these issues apply to
> a parallel worker -- the plan is already frozen before it can ever
> start, and it isn't going to be firing any triggers either.
>
> In particular, I think it's fine to get rid of
> ExecLockNonLeafAppendTables. In the parent, that's clearly useless code
> now: we have already locked *every* RTE_RELATION entry in the rangetable,
> either when the parser/rewriter/planner added the RTE to the list to begin
> with, or during AcquireExecutorLocks if the plan was retrieved from the
> plancache. In a child it seems unnecessary as long as we're locking the
> leaf rels we actually touch.
>
> Possibly, we might fix the problem of inadequate locking in worker
> processes by having them do the equivalent of AcquireExecutorLocks, ie
> just run through the whole RT list and lock everything. I think we'd soon
> end up doing that if we ever try to allow parallelization of non-read-only
> queries; but that's a long way off AFAIK. For read-only queries it seems
> like it'll be fine if we just rejigger ExecGetRangeTableRelation to take a
> lock when IsParallelWorker.

Okay, thanks for the explanation. It's now clearer to me why parallel
workers don't really need to lock non-leaf relations.

Thanks,
Amit