pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
21+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2024-03-06 099ca50bd4 c46817ee51 e82b64a92c 49b971298a 70a31629ae 69e8f9dadb Revert "Fix parallel-safety check of expressions and predicate for index builds"
2024-03-06 eae7be600b 4ec8f7708b 50b5913a87 56a8ab2fc6 b60d71c202 c0e0174c21 Fix parallel-safety check of expressions and predicate for index builds
* BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-01-29 07:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 21+ messages in thread
From: PG Bug reporting form @ 2024-01-29 07:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18314
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 16.1
Operating system: Ubuntu 22.04
Description:
When planner chooses to perform parallel index build, it doesn't
check whether functions used in an index expression or predicate are
parallel-safe. For example:
CREATE FUNCTION f() RETURNS int IMMUTABLE PARALLEL UNSAFE
AS $$
BEGIN
RETURN 0;
EXCEPTION WHEN OTHERS THEN
RETURN 1;
END$$ LANGUAGE plpgsql;
CREATE TABLE t(i int);
INSERT INTO t SELECT g FROM generate_series(1, 300000) g;
CREATE INDEX ON t((i + f()));
results in:
ERROR: cannot start subtransactions during a parallel operation
CONTEXT: PL/pgSQL function f() line 2 during statement block entry
parallel worker
Although with 200000 records, the index created with no error.
Reproduced on REL_12_STABLE .. master.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-01-29 14:36 Tender Wang <tndrwang@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 21+ messages in thread
From: Tender Wang @ 2024-01-29 14:36 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Thanks for reporting this issue.
I debug the process of creating index, and in plan_create_index_workers()
func, which use the planner to
decide if use parallel building index.
But the index expression passed to is_parallel_safe() has already
simplified by RelationGetIndexExpressions() func.
In this case, FunExpr node will be simplified to Const node. So the check
in is_parallel_safe() will return true.
The goal of plan_create_index_workers() is to decide whether it's safe to
build index parallelly. I think we just check the raw index
expression no need to simplify the index expression.
PG Bug reporting form <noreply@postgresql.org> 于2024年1月29日周一 19:02写道：
> The following bug has been logged on the website:
>
> Bug reference: 18314
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 16.1
> Operating system: Ubuntu 22.04
> Description:
>
> When planner chooses to perform parallel index build, it doesn't
> check whether functions used in an index expression or predicate are
> parallel-safe. For example:
> CREATE FUNCTION f() RETURNS int IMMUTABLE PARALLEL UNSAFE
> AS $$
> BEGIN
> RETURN 0;
> EXCEPTION WHEN OTHERS THEN
> RETURN 1;
> END$$ LANGUAGE plpgsql;
>
> CREATE TABLE t(i int);
> INSERT INTO t SELECT g FROM generate_series(1, 300000) g;
> CREATE INDEX ON t((i + f()));
>
> results in:
> ERROR: cannot start subtransactions during a parallel operation
> CONTEXT: PL/pgSQL function f() line 2 during statement block entry
> parallel worker
>
> Although with 200000 records, the index created with no error.
>
> Reproduced on REL_12_STABLE .. master.
>
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-07 11:54 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tender Wang @ 2024-02-07 11:54 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
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?
Tender Wang <tndrwang@gmail.com> 于2024年1月29日周一 22:36写道：
> Thanks for reporting this issue.
>
> I debug the process of creating index, and in plan_create_index_workers()
> func, which use the planner to
> decide if use parallel building index.
>
> But the index expression passed to is_parallel_safe() has already
> simplified by RelationGetIndexExpressions() func.
> In this case, FunExpr node will be simplified to Const node. So the check
> in is_parallel_safe() will return true.
>
> The goal of plan_create_index_workers() is to decide whether it's safe to
> build index parallelly. I think we just check the raw index
> expression no need to simplify the index expression.
>
>
> PG Bug reporting form <noreply@postgresql.org> 于2024年1月29日周一 19:02写道：
>
>> The following bug has been logged on the website:
>>
>> Bug reference: 18314
>> Logged by: Alexander Lakhin
>> Email address: exclusion@gmail.com
>> PostgreSQL version: 16.1
>> Operating system: Ubuntu 22.04
>> Description:
>>
>> When planner chooses to perform parallel index build, it doesn't
>> check whether functions used in an index expression or predicate are
>> parallel-safe. For example:
>> CREATE FUNCTION f() RETURNS int IMMUTABLE PARALLEL UNSAFE
>> AS $$
>> BEGIN
>> RETURN 0;
>> EXCEPTION WHEN OTHERS THEN
>> RETURN 1;
>> END$$ LANGUAGE plpgsql;
>>
>> CREATE TABLE t(i int);
>> INSERT INTO t SELECT g FROM generate_series(1, 300000) g;
>> CREATE INDEX ON t((i + f()));
>>
>> results in:
>> ERROR: cannot start subtransactions during a parallel operation
>> CONTEXT: PL/pgSQL function f() line 2 during statement block entry
>> parallel worker
>>
>> Although with 200000 records, the index created with no error.
>>
>> Reproduced on REL_12_STABLE .. master.
>>
>>
>
> --
> Tender Wang
> OpenPie: https://en.openpie.com/
>
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
0001-Fix-parallel-index-build-failed.patch (application/octet-stream, 12.7K) download
From 4aba1a446df1233cdc433ae44095d3a0c57d3b5b Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Wed, 7 Feb 2024 19:05:18 +0800
Subject: [PATCH] Fix parallel index build failed.
We will check whether it is safe to build btree index parallelly. But the
index expression or predicate which we check is modified not raw expression
or predicate. For example, function in expression may be optimized to const.
This optimization will miss function PARALLEL SAFE option, which will make
planner choose to build index parallelly. If function includes EXCEPTION, that
will report error.
Function containing an EXCEPTION clause effectively forms a subtransaction.
Now we cannot start subtransactions during a parallel operation
For whether to parallely building index, we need the raw index expression.
The RelationGetIndexExpressions() actually returns optimized expression not
raw expression. We choose to add a bool argument to RelationGetIndexExpressions()
not to write a new function which just return index raw expression.
---
src/backend/access/spgist/spgutils.c | 2 +-
src/backend/catalog/index.c | 4 ++--
src/backend/commands/matview.c | 2 +-
src/backend/commands/tablecmds.c | 4 ++--
src/backend/executor/execIndexing.c | 2 +-
src/backend/optimizer/plan/planner.c | 4 ++--
src/backend/optimizer/util/plancat.c | 8 ++++----
src/backend/parser/parse_utilcmd.c | 4 ++--
src/backend/utils/cache/relcache.c | 12 ++++++++++--
src/include/utils/relcache.h | 4 ++--
src/test/regress/expected/btree_index.out | 15 +++++++++++++++
src/test/regress/sql/btree_index.sql | 18 ++++++++++++++++++
12 files changed, 60 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 5b5e6e82d3..7acdc3a416 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -137,7 +137,7 @@ GetIndexInputType(Relation index, AttrNumber indexcol)
if (index->rd_indexprs)
indexprs = index->rd_indexprs;
else
- indexprs = RelationGetIndexExpressions(index);
+ indexprs = RelationGetIndexExpressions(index, true);
indexpr_item = list_head(indexprs);
for (int i = 1; i <= index->rd_index->indnkeyatts; i++)
{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 4b88a9cb87..d49b86a7c8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2455,8 +2455,8 @@ BuildIndexInfo(Relation index)
ii = makeIndexInfo(indexStruct->indnatts,
indexStruct->indnkeyatts,
index->rd_rel->relam,
- RelationGetIndexExpressions(index),
- RelationGetIndexPredicate(index),
+ RelationGetIndexExpressions(index, true),
+ RelationGetIndexPredicate(index, true),
indexStruct->indisunique,
indexStruct->indnullsnotdistinct,
indexStruct->indisready,
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 59920ced83..847f34b021 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -898,7 +898,7 @@ is_usable_unique_index(Relation indexRel)
indexStruct->indimmediate &&
indexRel->rd_rel->relam == BTREE_AM_OID &&
indexStruct->indisvalid &&
- RelationGetIndexPredicate(indexRel) == NIL &&
+ RelationGetIndexPredicate(indexRel, true) == NIL &&
indexStruct->indnatts > 0)
{
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9f51696740..d9bc9b7693 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17164,13 +17164,13 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
errmsg("cannot use non-immediate index \"%s\" as replica identity",
RelationGetRelationName(indexRel))));
/* Expression indexes aren't supported. */
- if (RelationGetIndexExpressions(indexRel) != NIL)
+ if (RelationGetIndexExpressions(indexRel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot use expression index \"%s\" as replica identity",
RelationGetRelationName(indexRel))));
/* Predicate indexes aren't supported. */
- if (RelationGetIndexPredicate(indexRel) != NIL)
+ if (RelationGetIndexPredicate(indexRel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot use partial index \"%s\" as replica identity",
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 9f05b3654c..ad6e5ab23a 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -1045,7 +1045,7 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
* If we find any matching Vars, don't pass hint for index. Otherwise
* pass hint.
*/
- idxExprs = RelationGetIndexExpressions(indexRelation);
+ idxExprs = RelationGetIndexExpressions(indexRelation, true);
hasexpression = index_expression_changed_walker((Node *) idxExprs,
allUpdatedCols);
list_free(idxExprs);
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index acc324122f..9a733e3148 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6670,8 +6670,8 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
* safe.
*/
if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
- !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index)) ||
- !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index)))
+ !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index, false)) ||
+ !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index, false)))
{
parallel_workers = 0;
goto done;
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index b933eefa64..0a503269e1 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -434,8 +434,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
* correct varno for the parent relation, so that they match up
* correctly against qual clauses.
*/
- info->indexprs = RelationGetIndexExpressions(indexRelation);
- info->indpred = RelationGetIndexPredicate(indexRelation);
+ info->indexprs = RelationGetIndexExpressions(indexRelation, true);
+ info->indpred = RelationGetIndexPredicate(indexRelation, true);
if (info->indexprs && varno != 1)
ChangeVarNodes((Node *) info->indexprs, 1, varno, 0);
if (info->indpred && varno != 1)
@@ -847,7 +847,7 @@ infer_arbiter_indexes(PlannerInfo *root)
goto next;
/* Expression attributes (if any) must match */
- idxExprs = RelationGetIndexExpressions(idxRel);
+ idxExprs = RelationGetIndexExpressions(idxRel, true);
foreach(el, onconflict->arbiterElems)
{
InferenceElem *elem = (InferenceElem *) lfirst(el);
@@ -898,7 +898,7 @@ infer_arbiter_indexes(PlannerInfo *root)
* If it's a partial index, its predicate must be implied by the ON
* CONFLICT's WHERE clause.
*/
- predExprs = RelationGetIndexPredicate(idxRel);
+ predExprs = RelationGetIndexPredicate(idxRel, true);
if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere, false))
goto next;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 56ac4f516e..928a0d0869 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2413,14 +2413,14 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
errdetail("Cannot create a primary key or unique constraint using such an index."),
parser_errposition(cxt->pstate, constraint->location)));
- if (RelationGetIndexExpressions(index_rel) != NIL)
+ if (RelationGetIndexExpressions(index_rel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("index \"%s\" contains expressions", index_name),
errdetail("Cannot create a primary key or unique constraint using such an index."),
parser_errposition(cxt->pstate, constraint->location)));
- if (RelationGetIndexPredicate(index_rel) != NIL)
+ if (RelationGetIndexPredicate(index_rel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is a partial index", index_name),
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..2b5a131038 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5007,7 +5007,7 @@ RelationGetReplicaIndex(Relation relation)
* disappear due to relcache invalidation.)
*/
List *
-RelationGetIndexExpressions(Relation relation)
+RelationGetIndexExpressions(Relation relation, bool need_modify)
{
List *result;
Datum exprsDatum;
@@ -5038,6 +5038,10 @@ RelationGetIndexExpressions(Relation relation)
result = (List *) stringToNode(exprsString);
pfree(exprsString);
+ /* If we need raw index expression, return */
+ if (!need_modify)
+ return result;
+
/*
* Run the expressions through eval_const_expressions. This is not just an
* optimization, but is necessary, because the planner will be comparing
@@ -5120,7 +5124,7 @@ RelationGetDummyIndexExpressions(Relation relation)
* disappear due to relcache invalidation.)
*/
List *
-RelationGetIndexPredicate(Relation relation)
+RelationGetIndexPredicate(Relation relation, bool need_modify)
{
List *result;
Datum predDatum;
@@ -5151,6 +5155,10 @@ RelationGetIndexPredicate(Relation relation)
result = (List *) stringToNode(predString);
pfree(predString);
+ /* If we need a raw index predicate, return */
+ if (!need_modify)
+ return result;
+
/*
* Run the expression through const-simplification and canonicalization.
* This is not just an optimization, but is necessary, because the planner
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 18c32ea700..d1529f7a69 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -48,9 +48,9 @@ extern List *RelationGetIndexList(Relation relation);
extern List *RelationGetStatExtList(Relation relation);
extern Oid RelationGetPrimaryKeyIndex(Relation relation);
extern Oid RelationGetReplicaIndex(Relation relation);
-extern List *RelationGetIndexExpressions(Relation relation);
+extern List *RelationGetIndexExpressions(Relation relation, bool need_modify);
extern List *RelationGetDummyIndexExpressions(Relation relation);
-extern List *RelationGetIndexPredicate(Relation relation);
+extern List *RelationGetIndexPredicate(Relation relation, bool need_modify);
extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy);
/*
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index 8311a03c3d..c96c908d41 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -434,3 +434,18 @@ ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_part_idx"
DETAIL: This operation is not supported for partitioned indexes.
DROP TABLE btree_part;
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+CREATE TABLE btree_para_bld(i int);
+INSERT INTO btree_para_bld SELECT g FROM generate_series(1, 300000) g;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+--Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index ef84354234..ac687a8ad7 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -267,3 +267,21 @@ CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id);
CREATE INDEX btree_part_idx ON btree_part(id);
ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
DROP TABLE btree_part;
+
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+
+CREATE TABLE btree_para_bld(i int);
+INSERT INTO btree_para_bld SELECT g FROM generate_series(1, 300000) g;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+
+--Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
--
2.25.1
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-07 15:43 jian he <jian.universality@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: jian he @ 2024-02-07 15:43 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Wed, Feb 7, 2024 at 7:54 PM Tender Wang <tndrwang@gmail.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?
>
/home/jian/Downloads/patches/0001-Fix-parallel-index-build-failed.patch:273:
trailing whitespace.
BEGIN
warning: 1 line adds whitespace errors.
+CREATE TABLE btree_para_bld(i int);
+INSERT INTO btree_para_bld SELECT g FROM generate_series(1, 300000) g;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
you don't need an insert of 300000 rows to test it.
you can just
CREATE TABLE btree_para_bld(i int);
ALTER TABLE btree_para_bld SET (parallel_workers = 4);
SET max_parallel_maintenance_workers TO 4;
CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
reset max_parallel_maintenance_workers;
demo: https://dbfiddle.uk/HpLKN6zj
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-08 06:17 jian he <jian.universality@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: jian he @ 2024-02-08 06:17 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Wed, Feb 7, 2024 at 11:43 PM jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Feb 7, 2024 at 7:54 PM Tender Wang <tndrwang@gmail.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?
> >
based on index_build's ereport(DEBUG1...) output information
you patch disabled parallel index build for parallel safe function.
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.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-23 12:21 jian he <jian.universality@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: jian he @ 2024-02-23 12:21 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Thu, Feb 8, 2024 at 2:17 PM jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Feb 7, 2024 at 11:43 PM jian he <jian.universality@gmail.com> wrote:
> >
> > On Wed, Feb 7, 2024 at 7:54 PM Tender Wang <tndrwang@gmail.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.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-27 10:17 jian he <jian.universality@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: jian he @ 2024-02-27 10:17 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi.
one minor issue.
-RelationGetIndexPredicate(Relation relation)
+RelationGetIndexPredicate(Relation relation, bool need_modify)
need_modify name may not be that good IMHO, but so far I can only come
up with are "raw_expr", "is_raw".
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-29 07:08 Tender Wang <tndrwang@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tender Wang @ 2024-02-29 07:08 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
jian he <jian.universality@gmail.com> 于2024年2月27日周二 18:17写道：
> Hi.
> one minor issue.
>
> -RelationGetIndexPredicate(Relation relation)
> +RelationGetIndexPredicate(Relation relation, bool need_modify)
> need_modify name may not be that good IMHO, but so far I can only come
> up with are "raw_expr", "is_raw".
>
Hi jian he,
I'm sorry for the delayed response. Thanks for your reviewing, and the
new v2 patch include your advices.
As no others reply to this patch. I would add it to the 2024-03 commitfest.
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
v2-0001-Fix-parallel-index-build-failed.patch (application/octet-stream, 13.2K) download
From 0a0b3b5170dfbade51021d9c146a9586bbe75a37 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Thu, 29 Feb 2024 15:01:38 +0800
Subject: [PATCH v2] Fix parallel index build failed.
We will check whether it is safe to build btree index parallelly. But the
index expression or predicate which we check is modified not raw expression
or predicate. For example, function in expression may be optimized to const.
This optimization will miss function PARALLEL SAFE option, which will make
planner choose to build index parallelly. If function includes EXCEPTION, that
will report error.
Function containing an EXCEPTION clause effectively forms a subtransaction.
Now we cannot start subtransactions during a parallel operation
For whether to parallely building index, we need the raw index expression.
The RelationGetIndexExpressions() actually returns optimized expression not
raw expression. We choose to add a bool argument to RelationGetIndexExpressions()
not to write a new function which just return index raw expression.
---
src/backend/access/spgist/spgutils.c | 2 +-
src/backend/catalog/index.c | 4 ++--
src/backend/commands/matview.c | 2 +-
src/backend/commands/tablecmds.c | 4 ++--
src/backend/executor/execIndexing.c | 2 +-
src/backend/optimizer/plan/planner.c | 9 +++++++--
src/backend/optimizer/util/plancat.c | 8 ++++----
src/backend/parser/parse_utilcmd.c | 4 ++--
src/backend/utils/cache/relcache.c | 12 ++++++++++--
src/include/utils/relcache.h | 4 ++--
src/test/regress/expected/btree_index.out | 17 +++++++++++++++++
src/test/regress/sql/btree_index.sql | 21 +++++++++++++++++++++
12 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 5b5e6e82d3..7acdc3a416 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -137,7 +137,7 @@ GetIndexInputType(Relation index, AttrNumber indexcol)
if (index->rd_indexprs)
indexprs = index->rd_indexprs;
else
- indexprs = RelationGetIndexExpressions(index);
+ indexprs = RelationGetIndexExpressions(index, true);
indexpr_item = list_head(indexprs);
for (int i = 1; i <= index->rd_index->indnkeyatts; i++)
{
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 4b88a9cb87..d49b86a7c8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2455,8 +2455,8 @@ BuildIndexInfo(Relation index)
ii = makeIndexInfo(indexStruct->indnatts,
indexStruct->indnkeyatts,
index->rd_rel->relam,
- RelationGetIndexExpressions(index),
- RelationGetIndexPredicate(index),
+ RelationGetIndexExpressions(index, true),
+ RelationGetIndexPredicate(index, true),
indexStruct->indisunique,
indexStruct->indnullsnotdistinct,
indexStruct->indisready,
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 59920ced83..847f34b021 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -898,7 +898,7 @@ is_usable_unique_index(Relation indexRel)
indexStruct->indimmediate &&
indexRel->rd_rel->relam == BTREE_AM_OID &&
indexStruct->indisvalid &&
- RelationGetIndexPredicate(indexRel) == NIL &&
+ RelationGetIndexPredicate(indexRel, true) == NIL &&
indexStruct->indnatts > 0)
{
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f798794556..8db9790315 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17172,13 +17172,13 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
errmsg("cannot use non-immediate index \"%s\" as replica identity",
RelationGetRelationName(indexRel))));
/* Expression indexes aren't supported. */
- if (RelationGetIndexExpressions(indexRel) != NIL)
+ if (RelationGetIndexExpressions(indexRel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot use expression index \"%s\" as replica identity",
RelationGetRelationName(indexRel))));
/* Predicate indexes aren't supported. */
- if (RelationGetIndexPredicate(indexRel) != NIL)
+ if (RelationGetIndexPredicate(indexRel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot use partial index \"%s\" as replica identity",
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 9f05b3654c..ad6e5ab23a 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -1045,7 +1045,7 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate,
* If we find any matching Vars, don't pass hint for index. Otherwise
* pass hint.
*/
- idxExprs = RelationGetIndexExpressions(indexRelation);
+ idxExprs = RelationGetIndexExpressions(indexRelation, true);
hasexpression = index_expression_changed_walker((Node *) idxExprs,
allUpdatedCols);
list_free(idxExprs);
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index be4e182869..3951d1924e 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6674,10 +6674,15 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
* Currently, parallel workers can't access the leader's temporary tables.
* Furthermore, any index predicate or index expressions must be parallel
* safe.
+ *
+ * We would check the raw index expression not optimized expression.
+ * ReationGetIndexExpression() will optimize the index expression even though
+ * user provide PARALLEL UNSAFE label to the expression.
+ * It will report error in some cases. So we pass FALSE to function.
*/
if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
- !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index)) ||
- !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index)))
+ !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index, false)) ||
+ !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index, false)))
{
parallel_workers = 0;
goto done;
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index b933eefa64..0a503269e1 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -434,8 +434,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
* correct varno for the parent relation, so that they match up
* correctly against qual clauses.
*/
- info->indexprs = RelationGetIndexExpressions(indexRelation);
- info->indpred = RelationGetIndexPredicate(indexRelation);
+ info->indexprs = RelationGetIndexExpressions(indexRelation, true);
+ info->indpred = RelationGetIndexPredicate(indexRelation, true);
if (info->indexprs && varno != 1)
ChangeVarNodes((Node *) info->indexprs, 1, varno, 0);
if (info->indpred && varno != 1)
@@ -847,7 +847,7 @@ infer_arbiter_indexes(PlannerInfo *root)
goto next;
/* Expression attributes (if any) must match */
- idxExprs = RelationGetIndexExpressions(idxRel);
+ idxExprs = RelationGetIndexExpressions(idxRel, true);
foreach(el, onconflict->arbiterElems)
{
InferenceElem *elem = (InferenceElem *) lfirst(el);
@@ -898,7 +898,7 @@ infer_arbiter_indexes(PlannerInfo *root)
* If it's a partial index, its predicate must be implied by the ON
* CONFLICT's WHERE clause.
*/
- predExprs = RelationGetIndexPredicate(idxRel);
+ predExprs = RelationGetIndexPredicate(idxRel, true);
if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere, false))
goto next;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index c7efd8d8ce..07dab10a4f 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2422,14 +2422,14 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
errdetail("Cannot create a primary key or unique constraint using such an index."),
parser_errposition(cxt->pstate, constraint->location)));
- if (RelationGetIndexExpressions(index_rel) != NIL)
+ if (RelationGetIndexExpressions(index_rel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("index \"%s\" contains expressions", index_name),
errdetail("Cannot create a primary key or unique constraint using such an index."),
parser_errposition(cxt->pstate, constraint->location)));
- if (RelationGetIndexPredicate(index_rel) != NIL)
+ if (RelationGetIndexPredicate(index_rel, true) != NIL)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is a partial index", index_name),
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 50acae4529..002c8b25f2 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5008,7 +5008,7 @@ RelationGetReplicaIndex(Relation relation)
* disappear due to relcache invalidation.)
*/
List *
-RelationGetIndexExpressions(Relation relation)
+RelationGetIndexExpressions(Relation relation, bool get_raw_expr)
{
List *result;
Datum exprsDatum;
@@ -5039,6 +5039,10 @@ RelationGetIndexExpressions(Relation relation)
result = (List *) stringToNode(exprsString);
pfree(exprsString);
+ /* Return, if we need raw index expression */
+ if (!get_raw_expr)
+ return result;
+
/*
* Run the expressions through eval_const_expressions. This is not just an
* optimization, but is necessary, because the planner will be comparing
@@ -5121,7 +5125,7 @@ RelationGetDummyIndexExpressions(Relation relation)
* disappear due to relcache invalidation.)
*/
List *
-RelationGetIndexPredicate(Relation relation)
+RelationGetIndexPredicate(Relation relation, bool get_raw_expr)
{
List *result;
Datum predDatum;
@@ -5152,6 +5156,10 @@ RelationGetIndexPredicate(Relation relation)
result = (List *) stringToNode(predString);
pfree(predString);
+ /* Return, if we need a raw index predicate */
+ if (!get_raw_expr)
+ return result;
+
/*
* Run the expression through const-simplification and canonicalization.
* This is not just an optimization, but is necessary, because the planner
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 18c32ea700..f1c7b0d2e5 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -48,9 +48,9 @@ extern List *RelationGetIndexList(Relation relation);
extern List *RelationGetStatExtList(Relation relation);
extern Oid RelationGetPrimaryKeyIndex(Relation relation);
extern Oid RelationGetReplicaIndex(Relation relation);
-extern List *RelationGetIndexExpressions(Relation relation);
+extern List *RelationGetIndexExpressions(Relation relation, bool get_raw_expr);
extern List *RelationGetDummyIndexExpressions(Relation relation);
-extern List *RelationGetIndexPredicate(Relation relation);
+extern List *RelationGetIndexPredicate(Relation relation, bool get_raw_expr);
extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy);
/*
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index 8311a03c3d..29a2903d80 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -434,3 +434,20 @@ ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_part_idx"
DETAIL: This operation is not supported for partitioned indexes.
DROP TABLE btree_part;
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+CREATE TABLE btree_para_bld(i int);
+ALTER TABLE btree_para_bld SET (parallel_workers = 4);
+SET max_parallel_maintenance_workers TO 4;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+-- Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+RESET max_parallel_maintenance_workers;
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index ef84354234..a054d106fb 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -267,3 +267,24 @@ CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id);
CREATE INDEX btree_part_idx ON btree_part(id);
ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
DROP TABLE btree_part;
+
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+
+CREATE TABLE btree_para_bld(i int);
+ALTER TABLE btree_para_bld SET (parallel_workers = 4);
+SET max_parallel_maintenance_workers TO 4;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+
+-- Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+
+RESET max_parallel_maintenance_workers;
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
--
2.25.1
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-02-29 09:17 jian he <jian.universality@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: jian he @ 2024-02-29 09:17 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Thu, Feb 29, 2024 at 3:08 PM Tender Wang <tndrwang@gmail.com> wrote:
>
> Hi jian he,
> I'm sorry for the delayed response. Thanks for your reviewing, and the new v2 patch include your advices.
>
> As no others reply to this patch. I would add it to the 2024-03 commitfest.
> --
`
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;
}
`
I refactored the comments for the code above.
Feel free to change it.
I also toggled {RelationGetIndexExpressions,
RelationGetIndexPredicate}'s second argument
from false to true where it's false,
true to false where it's true.
I guess it's more intuitive to do
`
if (get_raw_expr)
return result;
`
since at that time, the `result` is a raw expression.
Overall it looks good.
Attachments:
v2-0001-minor-refactor-based-on-v2.no-cfbot (application/octet-stream, 8.7K) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-05 03:44 Michael Paquier <michael@paquier.xyz>
parent: jian he <jian.universality@gmail.com>
0 siblings, 3 replies; 21+ messages in thread
From: Michael Paquier @ 2024-03-05 03:44 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
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.
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%40ma...
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-05 06:21 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
2 siblings, 0 replies; 21+ messages in thread
From: Tender Wang @ 2024-03-05 06:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
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().
>
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%40ma...
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/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-05 08:24 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
2 siblings, 0 replies; 21+ messages in thread
From: Tender Wang @ 2024-03-05 08:24 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
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%40ma...
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-05 11:49 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
2 siblings, 1 reply; 21+ messages in thread
From: Tender Wang @ 2024-03-05 11:49 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
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.
>
> 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%40ma...
> --
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
v3-0001-Fix-CREATE-INDEX-failed-due-to-parallel-unsafe-fu.patch (application/octet-stream, 6.0K) download
From f5a4e1129ac42026a9fb4992e0f21823062e89d9 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Tue, 5 Mar 2024 19:20:13 +0800
Subject: [PATCH v3] Fix CREATE INDEX failed due to parallel unsafe function.
Currently, the expression passed to is_parallel_safe() is created from
RelationGetIndexExpressions(). In RelationGetIndexExpressions(), it will
flatten the raw index expression if possible. It will ok in most cases, because
index expression usually only include column reference.
If index expressions include function, which is defined with PARALLEL UNSAFE label.
And the UDF is so simple that RelationGetIndexExpressions() will flatten it. It will
report error if building index in parallel mode. For example, the UDF contain EXCEPTION
statement.
The solution is to pass the raw index expression to is_parallel_safe(). The raw
expression should get from syscache not relcache.
---
src/backend/optimizer/plan/planner.c | 4 +--
src/backend/utils/cache/relcache.c | 43 +++++++++++++++++++++++
src/include/utils/relcache.h | 1 +
src/test/regress/expected/btree_index.out | 17 +++++++++
src/test/regress/sql/btree_index.sql | 21 +++++++++++
5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index ac97575453..2ab06f7573 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6669,8 +6669,8 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
* safe.
*/
if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
- !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index)) ||
- !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index)))
+ !is_parallel_safe(root, (Node *) RelationGetIndexClause(index, Anum_pg_index_indexprs)) ||
+ !is_parallel_safe(root, (Node *) RelationGetIndexClause(index, Anum_pg_index_indpred)))
{
parallel_workers = 0;
goto done;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 8cb88454c1..23a47e247a 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5057,6 +5057,49 @@ RelationGetIndexExpressions(Relation relation)
return result;
}
+/*
+ * get_raw_index_clause
+ * Return the raw index expression or predicate
+ *
+ * The att_num should be Anum_pg_index_indexprs or Anum_pg_index_indpred.
+ */
+List *
+RelationGetIndexClause(Relation relation, int attnum)
+{
+ List *result;
+ HeapTuple tuple;
+ Datum datum;
+ bool isnull;
+ char *attString;
+
+ Assert(attnum == Anum_pg_index_indexprs ||
+ attnum == Anum_pg_index_indpred);
+
+ tuple = SearchSysCache1(INDEXRELID,
+ ObjectIdGetDatum(RelationGetRelid(relation)));
+
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u",
+ RelationGetRelid(relation));
+
+ /* Extract raw node tree(s) from index tuple. */
+ datum = SysCacheGetAttr(INDEXRELID, tuple, attnum, &isnull);
+
+ if (isnull)
+ {
+ ReleaseSysCache(tuple);
+ return NIL;
+ }
+
+ attString = TextDatumGetCString(datum);
+ result = (List *) stringToNode(attString);
+ pfree(attString);
+
+ ReleaseSysCache(tuple);
+
+ return result;
+}
+
/*
* RelationGetDummyIndexExpressions -- get dummy expressions for an index
*
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 18c32ea700..83dfdc0dda 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -52,6 +52,7 @@ extern List *RelationGetIndexExpressions(Relation relation);
extern List *RelationGetDummyIndexExpressions(Relation relation);
extern List *RelationGetIndexPredicate(Relation relation);
extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy);
+extern List *RelationGetIndexClause(Relation relation, int attnum);
/*
* Which set of columns to return by RelationGetIndexAttrBitmap.
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index 8311a03c3d..29a2903d80 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -434,3 +434,20 @@ ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_part_idx"
DETAIL: This operation is not supported for partitioned indexes.
DROP TABLE btree_part;
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+CREATE TABLE btree_para_bld(i int);
+ALTER TABLE btree_para_bld SET (parallel_workers = 4);
+SET max_parallel_maintenance_workers TO 4;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+-- Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+RESET max_parallel_maintenance_workers;
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index ef84354234..a054d106fb 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -267,3 +267,24 @@ CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id);
CREATE INDEX btree_part_idx ON btree_part(id);
ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
DROP TABLE btree_part;
+
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+
+CREATE TABLE btree_para_bld(i int);
+ALTER TABLE btree_para_bld SET (parallel_workers = 4);
+SET max_parallel_maintenance_workers TO 4;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+
+-- Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+
+RESET max_parallel_maintenance_workers;
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
--
2.25.1
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-05 13:22 jian he <jian.universality@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: jian he @ 2024-03-05 13:22 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>
On Tue, Mar 5, 2024 at 7:49 PM Tender Wang <tndrwang@gmail.com> wrote:
>
>>
>> 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?
in RelationGetIndexClause to, I think you can use the following to
save a SearchSysCache1 cycle?
if (relation->rd_indextuple == NULL ||
heap_attisnull(relation->rd_indextuple, Anum_pg_index_indexprs, NULL))
return NIL;
or
if (relation->rd_indextuple == NULL ||
heap_attisnull(relation->rd_indextuple, Anum_pg_index_indpred, NULL))
return NIL;
main question would be why not two functions,
like RelationGetIndexRawExpr(Relation relation),
RelationGetIndexRawPred(Relation relation)
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-05 22:45 Michael Paquier <michael@paquier.xyz>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2024-03-05 22:45 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>
On Tue, Mar 05, 2024 at 09:22:08PM +0800, jian he wrote:
> in RelationGetIndexClause to, I think you can use the following to
> save a SearchSysCache1 cycle?
> if (relation->rd_indextuple == NULL ||
> heap_attisnull(relation->rd_indextuple, Anum_pg_index_indexprs, NULL))
> return NIL;
>
> or
> if (relation->rd_indextuple == NULL ||
> heap_attisnull(relation->rd_indextuple, Anum_pg_index_indpred, NULL))
> return NIL;
Don't think so. The point is to not rely on the relcache at all to
retrieve this information.
> main question would be why not two functions,
> like RelationGetIndexRawExpr(Relation relation),
> RelationGetIndexRawPred(Relation relation)
This comes down to if it is clean to have references to the catalog
pg_index in the planner, which is not the case yet so my take is that
two functions is much cleaner even if both return a List.
Anyway, why do you insist in putting the new functions in relcache.c?
I would suggest to move that to lsyscache.c instead, close to
get_index_column_opclass where there are routines for the syscache of
pg_index. It would be possible to reuse that in the reindex code, for
example.
The patch should add a comment in in plan_create_index_workers()
explaining why we care about raw expressions and indexes rather than
the relcache information.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-06 04:30 Tender Wang <tndrwang@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 21+ messages in thread
From: Tender Wang @ 2024-03-06 04:30 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
Michael Paquier <michael@paquier.xyz> 于2024年3月6日周三 06:45写道：
> On Tue, Mar 05, 2024 at 09:22:08PM +0800, jian he wrote:
> > in RelationGetIndexClause to, I think you can use the following to
> > save a SearchSysCache1 cycle?
> > if (relation->rd_indextuple == NULL ||
> > heap_attisnull(relation->rd_indextuple, Anum_pg_index_indexprs, NULL))
> > return NIL;
> >
> > or
> > if (relation->rd_indextuple == NULL ||
> > heap_attisnull(relation->rd_indextuple, Anum_pg_index_indpred, NULL))
> > return NIL;
>
> Don't think so. The point is to not rely on the relcache at all to
> retrieve this information.
>
> > main question would be why not two functions,
> > like RelationGetIndexRawExpr(Relation relation),
> > RelationGetIndexRawPred(Relation relation)
>
> This comes down to if it is clean to have references to the catalog
> pg_index in the planner, which is not the case yet so my take is that
> two functions is much cleaner even if both return a List.
>
> Anyway, why do you insist in putting the new functions in relcache.c?
> I would suggest to move that to lsyscache.c instead, close to
> get_index_column_opclass where there are routines for the syscache of
> pg_index. It would be possible to reuse that in the reindex code, for
> example.
>
Make sense.
Please review attached v4 patch.
>
> The patch should add a comment in in plan_create_index_workers()
> explaining why we care about raw expressions and indexes rather than
> the relcache information.
>
Added
--
> Michael
>
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
v4-0001-Fix-CREATE-INDEX-failed-due-to-parallel-unsafe-fu.patch (application/octet-stream, 7.2K) download
From 9f08325983cbc37f9a68ecf4cdab201f1b75dca0 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Wed, 6 Mar 2024 12:20:00 +0800
Subject: [PATCH v4] Fix CREATE INDEX failed due to parallel unsafe function.
Currently, the expression passed to is_parallel_safe() is created from
RelationGetIndexExpressions(). In RelationGetIndexExpressions(), it will
flatten the raw index expression if possible. It will ok in most cases, because
index expression usually only include column reference.
If index expressions include function, which is defined with PARALLEL UNSAFE label.
And the UDF is so simple that RelationGetIndexExpressions() will flatten it. It will
report error if building index in parallel mode. For example, the UDF contain EXCEPTION
statement.
The solution is to pass the raw index expression to is_parallel_safe(). The raw
expression should get from syscache not relcache.
---
src/backend/optimizer/plan/planner.c | 9 ++-
src/backend/utils/cache/lsyscache.c | 76 +++++++++++++++++++++++
src/include/utils/lsyscache.h | 2 +
src/test/regress/expected/btree_index.out | 17 +++++
src/test/regress/sql/btree_index.sql | 21 +++++++
5 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index ac97575453..0876179e83 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6667,10 +6667,15 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
* Currently, parallel workers can't access the leader's temporary tables.
* Furthermore, any index predicate or index expressions must be parallel
* safe.
+ *
+ * Note that we must get index expressions or predicate from syscache not
+ * relcache. The expressions stored in relcache may have been flattened in
+ * RelationGetIndexExpression(). The flattened expressions would lose PARALLEL
+ * UNSAFE label, which it is not what we want.
*/
if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
- !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index)) ||
- !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index)))
+ !is_parallel_safe(root, (Node *) get_index_expression(indexOid)) ||
+ !is_parallel_safe(root, (Node *) get_index_predicate(indexOid)))
{
parallel_workers = 0;
goto done;
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 6418d1c6eb..378764b0a8 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3502,6 +3502,82 @@ get_index_column_opclass(Oid index_oid, int attno)
return opclass;
}
+/*
+ * get_index_expression
+ *
+ * Given the index OID, return the index expression.
+ */
+List *
+get_index_expression(Oid index_oid)
+{
+ List *result;
+ HeapTuple tuple;
+ Datum exprDatum;
+ bool isnull;
+ char *exprString;
+
+ /* Fetch the pg_index tuple by the Oid of the index */
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ /* Extract raw index expression from index tuple */
+ exprDatum = SysCacheGetAttr(INDEXRELID, tuple,
+ Anum_pg_index_indexprs, &isnull);
+ if (isnull)
+ {
+ ReleaseSysCache(tuple);
+ return NIL;
+ }
+
+ exprString = TextDatumGetCString(exprDatum);
+ result = (List *) stringToNode(exprString);
+ pfree(exprString);
+
+ /* Clean up */
+ ReleaseSysCache(tuple);
+
+ return result;
+}
+
+/*
+ * get_index_predicate
+ *
+ * Given the index OID, return index predicate.
+ */
+List *
+get_index_predicate(Oid index_oid)
+{
+ List *result;
+ HeapTuple tuple;
+ Datum predDatum;
+ bool isnull;
+ char *predString;
+
+ /* Fetch the pg_index tuple by the Oid of the index */
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ /* Extract raw index predicate from index tuple */
+ predDatum = SysCacheGetAttr(INDEXRELID, tuple,
+ Anum_pg_index_indpred, &isnull);
+ if (isnull)
+ {
+ ReleaseSysCache(tuple);
+ return NIL;
+ }
+
+ predString = TextDatumGetCString(predDatum);
+ result = (List *) stringToNode(predString);
+ pfree(predString);
+
+ /* Clean up */
+ ReleaseSysCache(tuple);
+
+ return result;
+}
+
/*
* get_index_isreplident
*
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index e4a200b00e..cde6bb76ac 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -195,6 +195,8 @@ extern Oid get_range_collation(Oid rangeOid);
extern Oid get_range_multirange(Oid rangeOid);
extern Oid get_multirange_range(Oid multirangeOid);
extern Oid get_index_column_opclass(Oid index_oid, int attno);
+extern List *get_index_expression(Oid index_oid);
+extern List *get_index_predicate(Oid index_oid);
extern bool get_index_isreplident(Oid index_oid);
extern bool get_index_isvalid(Oid index_oid);
extern bool get_index_isclustered(Oid index_oid);
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index 8311a03c3d..29a2903d80 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -434,3 +434,20 @@ ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_part_idx"
DETAIL: This operation is not supported for partitioned indexes.
DROP TABLE btree_part;
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+CREATE TABLE btree_para_bld(i int);
+ALTER TABLE btree_para_bld SET (parallel_workers = 4);
+SET max_parallel_maintenance_workers TO 4;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+-- Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+RESET max_parallel_maintenance_workers;
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index ef84354234..a054d106fb 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -267,3 +267,24 @@ CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id);
CREATE INDEX btree_part_idx ON btree_part(id);
ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
DROP TABLE btree_part;
+
+-- Test index expression includes parallel unsafe function
+CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE
+AS $$
+BEGIN
+ RETURN 0;
+EXCEPTION WHEN OTHERS THEN
+ RETURN 1;
+END$$ LANGUAGE plpgsql;
+
+CREATE TABLE btree_para_bld(i int);
+ALTER TABLE btree_para_bld SET (parallel_workers = 4);
+SET max_parallel_maintenance_workers TO 4;
+CREATE INDEX ON btree_para_bld((i + para_unsafe_f()));
+
+-- Test index predicate includes parallel unsafe function
+CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f();
+
+RESET max_parallel_maintenance_workers;
+DROP TABLE btree_para_bld;
+DROP FUNCTION para_unsafe_f;
--
2.25.1
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-06 12:07 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tender Wang @ 2024-03-06 12:07 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
Thanks for pushing the patch.
I found a typos:
/*
+ * get_index_expressions
+ *
+ * Given the index OID, its a List of its expressions or NIL
if none.
+ */
"its" should be "return"?
Tender Wang <tndrwang@gmail.com> 于2024年3月6日周三 12:30写道：
>
>
> Michael Paquier <michael@paquier.xyz> 于2024年3月6日周三 06:45写道：
>
>> On Tue, Mar 05, 2024 at 09:22:08PM +0800, jian he wrote:
>> > in RelationGetIndexClause to, I think you can use the following to
>> > save a SearchSysCache1 cycle?
>> > if (relation->rd_indextuple == NULL ||
>> > heap_attisnull(relation->rd_indextuple, Anum_pg_index_indexprs, NULL))
>> > return NIL;
>> >
>> > or
>> > if (relation->rd_indextuple == NULL ||
>> > heap_attisnull(relation->rd_indextuple, Anum_pg_index_indpred, NULL))
>> > return NIL;
>>
>> Don't think so. The point is to not rely on the relcache at all to
>> retrieve this information.
>>
>> > main question would be why not two functions,
>> > like RelationGetIndexRawExpr(Relation relation),
>> > RelationGetIndexRawPred(Relation relation)
>>
>> This comes down to if it is clean to have references to the catalog
>> pg_index in the planner, which is not the case yet so my take is that
>> two functions is much cleaner even if both return a List.
>>
>> Anyway, why do you insist in putting the new functions in relcache.c?
>> I would suggest to move that to lsyscache.c instead, close to
>> get_index_column_opclass where there are routines for the syscache of
>> pg_index. It would be possible to reuse that in the reindex code, for
>> example.
>>
>
> Make sense.
> Please review attached v4 patch.
>
>>
>> The patch should add a comment in in plan_create_index_workers()
>> explaining why we care about raw expressions and indexes rather than
>> the relcache information.
>>
>
> Added
>
> --
>> Michael
>>
>
>
> --
> Tender Wang
> OpenPie: https://en.openpie.com/
>
--
Tender Wang
OpenPie: https://en.openpie.com/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-06 17:28 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2024-03-06 17:28 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
Tender Wang <tndrwang@gmail.com> writes:
> Thanks for pushing the patch.
My apologies for not having paid much attention to this thread
earlier, but ...
I don't like anything about the committed patch. It's a band-aid
that doesn't really solve the problem: if I simply declare the
function as PARALLEL SAFE, the failure comes right back. Of
course you can argue that then I lied about the parallel safety
of the function, but I think a far better answer is to make such
functions actually parallel safe, which I have a patch for
awaiting review in the current commitfest [1].
Moreover I think this breaks a fairly long-standing principle of
the planner, namely that if we can inline or const-fold a function
then its original markings no longer matter. People are likely
to be surprised that that no longer applies to parallel-safety
decisions, and even more surprised that it only doesn't apply
in this one narrow context.
I also believe that this has introduced new bugs, caused by
passing is_parallel_safe() raw parse-analysis output rather
than a tree that's been through eval_const_expressions.
For example, that processing is responsible for inserting
function default-argument expressions. If a default argument
is parallel-unsafe, this coding would fail to notice that.
(Compare commit 743ddafc7.) There might be other structures
in an un-simplified tree that is_parallel_safe() isn't expecting
and would spit up on.
As far as HEAD goes, I think we should revert this and then look
to get [1] committed. I'm unsure what to do in the back branches,
but given the lack of prior complaints I suspect we could get away
with just reverting and leaving the issue unfixed. (I wouldn't
propose back-patching [1], it seems too risky.)
If we want to do something about the more generic issue that
maybe const-folding will succeed in the leader process but
fail in workers, then I think the way to do that is to adjust
parallel index build so that the processed expression trees are
passed to the workers from the leader. That is how it's done
with regular query trees, and index build is evidently taking
a not-very-safe shortcut by not doing it like that. But I'm not
excited enough about the mostly-theoretical hazard to put any
work into that myself.
Another line of thought is whether we should just forbid any
parallel-unsafe functions in index expressions. If we continue
to allow them, I think we have to reject parallelizing any
query or DDL operation that touches the associated table at all,
because there's too much risk of a worker trying to evaluate
such an expression somewhere along the line. Again though,
given the lack of complaints I'm not excited about imposing
such a draconian policy. (The fact that functions are marked
parallel unsafe by default means that if we did, we'd almost
certainly break cases that are working fine for users today.)
regards, tom lane
[1] https://commitfest.postgresql.org/47/4776/
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-06 22:40 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2024-03-06 22:40 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
On Wed, Mar 06, 2024 at 12:28:11PM -0500, Tom Lane wrote:
> As far as HEAD goes, I think we should revert this and then look
> to get [1] committed.
>
> I'm unsure what to do in the back branches,
> but given the lack of prior complaints I suspect we could get away
> with just reverting and leaving the issue unfixed.
Still the issue fixed here and the issue of the other thread are
different? Relying on the relcache tricks the planner to spawn
workers for nothing because they would just fail if the internals of
the function are unsafe to run in the workers, and the function is
marked as such. In some cases users like to use properties as hints
to give to the planner about what to use or not. I'd like to believe
(and perhaps that's a cute thought) that functions are marked as
parallel-unsafe for a purpose, which is that they cannot run in
workers as they rely on a shared state. Whether the internals of the
functions are implemented correctly is up to the user. I mean, it's
perfectly possible to mark a function as immutable while its internals
are volatile, and I've seen people rely on that to push down clauses.
> (I wouldn't propose back-patching [1], it seems too risky.)
Agreed. Just looked at it and the change of blockState can be
sometimes tricky to get right. There are a bunch of internal
assumptions as far as I recall this area of the code.
> If we want to do something about the more generic issue that
> maybe const-folding will succeed in the leader process but
> fail in workers, then I think the way to do that is to adjust
> parallel index build so that the processed expression trees are
> passed to the workers from the leader. That is how it's done
> with regular query trees, and index build is evidently taking
> a not-very-safe shortcut by not doing it like that. But I'm not
> excited enough about the mostly-theoretical hazard to put any
> work into that myself.
You mean in the serialization and deserialization logic used when
workers are spawned and then let the workers complain when they try to
use such a function in a new path? That would be backpatchable, but
feels rather a bit invasive.
> Another line of thought is whether we should just forbid any
> parallel-unsafe functions in index expressions. If we continue
> to allow them, I think we have to reject parallelizing any
> query or DDL operation that touches the associated table at all,
> because there's too much risk of a worker trying to evaluate
> such an expression somewhere along the line. Again though,
> given the lack of complaints I'm not excited about imposing
> such a draconian policy. (The fact that functions are marked
> parallel unsafe by default means that if we did, we'd almost
> certainly break cases that are working fine for users today.)
Neither am I to restrict that moving forward. That could hurt
existing use cases that were allowed, still were able to work.
Saying that, seeing the lack of complaints and the fact that you're
not comfortable with that, I have no objections to revert the change
on all the branches. Will do so in a bit.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-06 23:01 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2024-03-06 23:01 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
Michael Paquier <michael@paquier.xyz> writes:
> Still the issue fixed here and the issue of the other thread are
> different? Relying on the relcache tricks the planner to spawn
> workers for nothing because they would just fail if the internals of
> the function are unsafe to run in the workers, and the function is
> marked as such.
Sure, but the current patch just trades one set of errors for others.
As I said, if we care about this problem then the right fix will
involve serializing/deserializing already-const-folded expression
trees instead of expecting that the workers can regenerate those.
I don't personally think it's worth the trouble, especially not if
we can get rid of the need for exception-using functions to be marked
parallel unsafe.
> In some cases users like to use properties as hints
> to give to the planner about what to use or not. I'd like to believe
> (and perhaps that's a cute thought) that functions are marked as
> parallel-unsafe for a purpose, which is that they cannot run in
> workers as they rely on a shared state. Whether the internals of the
> functions are implemented correctly is up to the user. I mean, it's
> perfectly possible to mark a function as immutable while its internals
> are volatile, and I've seen people rely on that to push down clauses.
It's the other direction that I'm concerned about: if we inline a SQL
function and find that its innards are less volatile than the original
marking, we are free to exploit that, and people will complain if we
don't. Similarly, if the innards are more parallel safe than the
original marking, we should be able to exploit that.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build
@ 2024-03-06 23:42 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 21+ messages in thread
From: Michael Paquier @ 2024-03-06 23:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs@lists.postgresql.org, Alexander Lakhin <exclusion@gmail.com>; jian he <jian.universality@gmail.com>
On Wed, Mar 06, 2024 at 06:01:50PM -0500, Tom Lane wrote:
> Sure, but the current patch just trades one set of errors for others.
> As I said, if we care about this problem then the right fix will
> involve serializing/deserializing already-const-folded expression
> trees instead of expecting that the workers can regenerate those.
> I don't personally think it's worth the trouble, especially not if
> we can get rid of the need for exception-using functions to be marked
> parallel unsafe.
I don't know if that's worth tackling either. It sounds like this is
just something we should do only if someone comes up with a good use
case for it, outside of the subtransaction handling in the workers.
Anyway, I've just reverted the change for now.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2024-03-06 23:42 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-29 07:00 BUG #18314: PARALLEL UNSAFE function does not prevent parallel index build PG Bug reporting form <noreply@postgresql.org>
2024-01-29 14:36 ` Tender Wang <tndrwang@gmail.com>
2024-02-07 11:54 ` Tender Wang <tndrwang@gmail.com>
2024-02-07 15:43 ` jian he <jian.universality@gmail.com>
2024-02-08 06:17 ` jian he <jian.universality@gmail.com>
2024-02-23 12:21 ` jian he <jian.universality@gmail.com>
2024-02-27 10:17 ` jian he <jian.universality@gmail.com>
2024-02-29 07:08 ` Tender Wang <tndrwang@gmail.com>
2024-02-29 09:17 ` jian he <jian.universality@gmail.com>
2024-03-05 03:44 ` Michael Paquier <michael@paquier.xyz>
2024-03-05 06:21 ` Tender Wang <tndrwang@gmail.com>
2024-03-05 08:24 ` Tender Wang <tndrwang@gmail.com>
2024-03-05 11:49 ` Tender Wang <tndrwang@gmail.com>
2024-03-05 13:22 ` jian he <jian.universality@gmail.com>
2024-03-05 22:45 ` Michael Paquier <michael@paquier.xyz>
2024-03-06 04:30 ` Tender Wang <tndrwang@gmail.com>
2024-03-06 12:07 ` Tender Wang <tndrwang@gmail.com>
2024-03-06 17:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-03-06 22:40 ` Michael Paquier <michael@paquier.xyz>
2024-03-06 23:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-03-06 23:42 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox