On Mon, Mar 8, 2021 at 6:25 PM Amit Langote <amitlangot...@gmail.com> wrote: > > A couple of things that look odd in v24-0001 (sorry if there were like > that from the beginning): > > +static bool > +target_rel_max_parallel_hazard(max_parallel_hazard_context *context) > +{ > + bool max_hazard_found; > + > + Relation targetRel; > + > + /* > + * The target table is already locked by the caller (this is done in the > + * parse/analyze phase), and remains locked until end-of-transaction. > + */ > + targetRel = table_open(context->target_rte->relid, > + context->target_rte->rellockmode); > > The comment seems to imply that the lock need not be retaken here, but > the code does. Maybe it's fine to pass NoLock here, but use > rellockmode when locking partitions, because they would not have been > locked by the parser/analyzer.
Actually, it was originally NoLock, but I think in your suggested changes (v15_delta.diff) to better integrate the extra parallel-safety checks into max_parallel_hazard(), you changed "NoLock" to "context->targetRTE->rellockmode".. Having said that, since the table is already locked, I think that using "context->target_rte->rellockmode" in this case just results in the lock reference count being incremented, so I doubt it really makes any difference, since it's locked until end-of-transaction. I'll revert it back to NoLock. >Which brings me to: > > +static bool > +target_rel_partitions_max_parallel_hazard(Relation rel, > + max_parallel_hazard_context > *context) > +{ > ... > + for (i = 0; i < pdesc->nparts; i++) > + { > + bool max_hazard_found; > + Relation part_rel; > + > + /* > + * The partition needs to be locked, and remain locked until > + * end-of-transaction to ensure its parallel-safety state is not > + * hereafter altered. > + */ > + part_rel = table_open(pdesc->oids[i], AccessShareLock); > > Not that I can prove there to be any actual hazard, but we tend to > avoid taking locks with different strengths in the same query as would > occur with this piece of code. We're locking the partition with > AccessShareLock here, but the executor will lock the partition with > the stronger RowExclusiveLock mode before trying to insert into it. > Better to use the stronger mode to begin with or just use the target > partitioned table's RTE's rellockmode which would be RowExclusiveLock. > You can see that that's what AcquireExecutorLocksOnPartitions() will > do in the generic plan case. > OK, I see what you mean, best to use the target partitioned table's RTE's rellockmode in this case then. I'll update the patch accordingly. Regards, Greg Nancarrow Fujitsu Australia