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().
>

Agreed! It is not friendly to others who maybe use these functions.

>
> 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.
>
> Is that possible? In this case, it is CREATE INDEX, the relation in
relcache should not have this index definition?
I try to create index on create table phase, but I failed. It looks like
index on expression only be created using CREATE INDEX statement.
Is there something I need to learn?

> 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.
>
> 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

In aboved link, I found that
"Even without that, changing the APIs of the functions you've touched
here seems pretty invasive." (Tom said that). It sounds sounds reasonable.

--
> Michael
>

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