Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build

Michael Paquier <michael(at)paquier(dot)xyz> 于2024年3月5日周二 11:45写道：

> On Thu, Feb 29, 2024 at 05:17:41PM +0800, jian he wrote:
> > if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
> > !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index,
> true)) ||
> > !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index, true)))
> > {
> > parallel_workers = 0;
> > goto done;
> > }
>
> Interesting bug.
>
> > Overall it looks good.
>
> I disagree with this idea. Your patch is creating a shortcut in the
> relcache code to retrieve data about index predicates and expressions
> that make it bypass the flattening that would be done in
> eval_const_expressions(). Giving the option to bypass that in the
> relcache is a very bad idea, because flattening of the expressions for
> the planner is not an option: it must happen. So, I fear that your
> patch could cause bugs if we introduce new code that calls
> RelationGetIndexExpressions() with an incorrect option. Not
> documenting the new option is not acceptable either, with only one
> comment added at the top of plan_create_index_workers().
>
> On top of that, your approach has two bugs:
> RelationGetIndexExpressions and RelationGetIndexPredicate would return
> the flattened information if available directly from the relcache, so
> even if you pass get_raw_expr=true you may finish with flattened
> expressions and/or predicates, facing the same issues.
>
> I think that the correct way to do that would be to get the
> information from the syscache when calculating the number of parallel
> workers to use for the parallel index builds, with SysCacheGetAttr(),
> for example. That would ensure that the expressions are not
> flattened, letting the relcache be.
>

I refactor the v2 version patch according to your advice.
Any thoughts?

> By the way, this reminds me of 7cce159349cc and its thread for a bug
> fix that I did for REINDEX CONCURRENTLY a couple of years ago. The
> mistake is mostly the same:
>
> https://www.postgresql.org/message-id/CA%2Bu7OA5Hp0ra235F3czPom_FyAd-3%2BXwSJmX95r1%2BsRPOJc9VQ%40mail.gmail.com
> --
> Michael
>

--
Tender Wang
OpenPie: https://en.openpie.com/