On Mon, Mar 8, 2021 at 12:55 PM Amit Langote <amitlangot...@gmail.com> wrote: > > Hi Amit, Greg, > > Sorry, I hadn't noticed last week that some questions were addressed to me. > > On Sat, Mar 6, 2021 at 7:19 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > Thanks, your changes look good to me. I went ahead and changed the > > patch to track the partitionOids once rather than in two separate > > lists and use that list in PlanCacheRelCallback to invalidate the > > plans. > > Not mixing the partition OIDs into relationOids seems fine to me. I > had considered that option too, but somehow forgot to mention it here. >
Okay, thanks for the confirmation. > 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. 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. > Both of your comments make sense to me. -- With Regards, Amit Kapila.