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