On 2018/10/07 3:59, Tom Lane wrote: > Amit Langote <langote_amit...@lab.ntt.co.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
