Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build - Mailing list pgsql-bugs

From Tender Wang
Subject Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
Date
Msg-id CAHewXNkaeMUZFac-UBim88xkP683o7px52oz-MXYN6n-r_ifBA@mail.gmail.com
Whole thread Raw
In response to Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build  (Michael Paquier <michael@paquier.xyz>)
List pgsql-bugs


Michael Paquier <michael@paquier.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.

Yeah,  I have carefully considered what you said. It is a bad idea to change the RelationGetIndexExpressions().
Even though it can fix this thread issue, but if new code calls RelationGetIndexExpressions() and
want to get the raw index expression, so it pass get_raw_expr=tree, it maybe get optimized index expression,  that could cause some bugs.
 
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.
 
The fix  here is to pass the raw index expression to is_parallel_safe().  Getting the information from the syscache sournds reasonable.

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/

pgsql-bugs by date:

Previous
From: Devrim Gündüz
Date:
Subject: Re: BUG #18378: postgis protobuf support inconsistent
Next
From: 费长红
Date:
Subject: Re: BUG #18371: There are wrong constraint residues when detach hash partiton concurrently