On Mon, Mar 8, 2021 at 12:55 PM Amit Langote <[email protected]> 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 <[email protected]> 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.
