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

On Thu, Feb 8, 2024 at 2:17 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Wed, Feb 7, 2024 at 11:43 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >
> > On Wed, Feb 7, 2024 at 7:54 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> > >
> > > In pg document, I found this:
> > >
> > > Also, a block containing an EXCEPTION clause effectively forms a subtransaction that can be rolled back without affecting the outer transaction.
> > >
> > > So it will report error for this case even though we replace PARALLEL UNSAFE with PARALLEL SAFE.
> > >
> > > I think if PARALLEL UNSAFE is specified by users, PG internal should not choose to build index parallel, even if the function is
> > > too simply that can be transform to Const node.
> > >
> > > Attached patch will fix Alexander reported issue. I choose pass the raw index expression to is_parallel_safe().
> > > The comments of RelationGetIndexExpressions() doesn't say it will return optimized expression. So I add a bool argument to RelationGetIndexExpressions().
> > > I don't decide to write a new function like RelationGetIndexRawExpression. I think the RelationGetIndexExpressions() is enough after adding a bool argument to indicate
> > > whether caller needing a raw index expression.
> > >
> > > But if users specify PARALLEL SAFE, like I said before, it also reports error. But I think it is another thing. Maybe it is reasonable?
> > >

I think it's reasonable.
It's the function owner's responsibility to specify PARALLEL safety correctly.

> after apply your patch:
> set log_min_messages to debug1;
> begin;
> drop table if exists btree_para_bld;
> CREATE TABLE btree_para_bld(i int);
> ALTER TABLE btree_para_bld SET (parallel_workers = 4);
> SET local max_parallel_maintenance_workers TO 4;
> CREATE or replace FUNCTION para_unsafe_f1() RETURNS int IMMUTABLE
> PARALLEL UNSAFE
> AS $$
> BEGIN
> RETURN 0;
> END$$ LANGUAGE plpgsql;
>
> CREATE or replace FUNCTION para_safe() RETURNS int IMMUTABLE PARALLEL UNSAFE
> AS $$
> BEGIN
> RETURN 0;
> END$$ LANGUAGE plpgsql;
> CREATE INDEX ON btree_para_bld((para_unsafe_f1()));
> CREATE INDEX ON btree_para_bld((para_safe()));
> commit;
>
> debug information:
> 2024-02-08 14:11:26.664 CST [33245] DEBUG: building index
> "btree_para_bld_para_unsafe_f1_idx" on table "btree_para_bld" serially
> 2024-02-08 14:11:26.664 CST [33245] DEBUG: index
> "btree_para_bld_para_unsafe_f1_idx" can safely use deduplication
> CREATE INDEX
> 2024-02-08 14:11:26.666 CST [33245] DEBUG: building index
> "btree_para_bld_para_safe_idx" on table "btree_para_bld" serially
> 2024-02-08 14:11:26.666 CST [33245] DEBUG: index
> "btree_para_bld_para_safe_idx" can safely use deduplication
> CREATE INDEX
>
> the problem entry point is_parallel_safe, i think.

I am sorry, this part was wrong.
Your patch works perfectly!

if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
!is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index, false)) ||
!is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index, false)))
{
parallel_workers = 0;
goto done;
}
add more comments in above code would be perfect.
As I mentioned before, your test case can be simplified.