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.
