Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit9bd0f74

Browse files
committed
Ensure we preprocess expressions before checking their volatility.
contain_mutable_functions and contain_volatile_functions givereliable answers only after expression preprocessing (specificallyeval_const_expressions). Some places understand this, but some didnot get the memo --- which is not entirely their fault, because theproblem is documented only in places far away from those functions.Introduce wrapper functions that allow doing the right thing easily,and add commentary in hopes of preventing future mistakes fromcopy-and-paste of code that's only conditionally safe.Two actual bugs of this ilk are fixed here. We failed to preprocesscolumn GENERATED expressions before checking mutability, so that thecode could fail to detect the use of a volatile functiondefault-argument expression, or it could reject a polymorphic functionthat is actually immutable on the datatype of interest. Likewise,column DEFAULT expressions weren't preprocessed before determining ifit's safe to apply the attmissingval mechanism. A false negativewould just result in an unnecessary table rewrite, but a falsepositive could allow the attmissingval mechanism to be used in a casewhere it should not be, resulting in unexpected initial values in anew column.In passing, re-order the steps in ComputePartitionAttrs so that itschecks for invalid column references are done before applyingexpression_planner, rather than after. The previous coding wouldnot complain if a partition expression contains a disallowed columnreference that gets optimized away by constant folding, which seemsto me to be a behavior we do not want.Per bug #18097 from Jim Keener. Back-patch to all supported versions.Discussion:https://postgr.es/m/18097-ebb179674f22932f@postgresql.org
1 parenta54129e commit9bd0f74

File tree

10 files changed

+141
-56
lines changed

10 files changed

+141
-56
lines changed

‎src/backend/catalog/heap.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2658,7 +2658,8 @@ AddRelationNewConstraints(Relation rel,
26582658
continue;
26592659

26602660
/* If the DEFAULT is volatile we cannot use a missing value */
2661-
if (colDef->missingMode&&contain_volatile_functions((Node*)expr))
2661+
if (colDef->missingMode&&
2662+
contain_volatile_functions_after_planning((Expr*)expr))
26622663
colDef->missingMode= false;
26632664

26642665
defOid=StoreAttrDefault(rel,colDef->attnum,expr,is_internal,
@@ -3093,9 +3094,11 @@ cookDefault(ParseState *pstate,
30933094

30943095
if (attgenerated)
30953096
{
3097+
/* Disallow refs to other generated columns */
30963098
check_nested_generated(pstate,expr);
30973099

3098-
if (contain_mutable_functions(expr))
3100+
/* Disallow mutable functions */
3101+
if (contain_mutable_functions_after_planning((Expr*)expr))
30993102
ereport(ERROR,
31003103
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
31013104
errmsg("generation expression is not immutable")));

‎src/backend/commands/copyfrom.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -755,6 +755,9 @@ CopyFrom(CopyFromState cstate)
755755
* Can't support multi-inserts if there are any volatile function
756756
* expressions in WHERE clause. Similarly to the trigger case above,
757757
* such expressions may query the table we're inserting into.
758+
*
759+
* Note: the whereClause was already preprocessed in DoCopy(), so it's
760+
* okay to use contain_volatile_functions() directly.
758761
*/
759762
insertMethod=CIM_SINGLE;
760763
}
@@ -1454,7 +1457,8 @@ BeginCopyFrom(ParseState *pstate,
14541457
* known to be safe for use with the multi-insert
14551458
* optimization. Hence we use this special case function
14561459
* checker rather than the standard check for
1457-
* contain_volatile_functions().
1460+
* contain_volatile_functions(). Note also that we already
1461+
* ran the expression through expression_planner().
14581462
*/
14591463
if (!volatile_defexprs)
14601464
volatile_defexprs=contain_volatile_functions_not_nextval((Node*)defexpr);

‎src/backend/commands/indexcmds.c

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1720,33 +1720,6 @@ DefineIndex(Oid relationId,
17201720
}
17211721

17221722

1723-
/*
1724-
* CheckMutability
1725-
*Test whether given expression is mutable
1726-
*/
1727-
staticbool
1728-
CheckMutability(Expr*expr)
1729-
{
1730-
/*
1731-
* First run the expression through the planner. This has a couple of
1732-
* important consequences. First, function default arguments will get
1733-
* inserted, which may affect volatility (consider "default now()").
1734-
* Second, inline-able functions will get inlined, which may allow us to
1735-
* conclude that the function is really less volatile than it's marked. As
1736-
* an example, polymorphic functions must be marked with the most volatile
1737-
* behavior that they have for any input type, but once we inline the
1738-
* function we may be able to conclude that it's not so volatile for the
1739-
* particular input type we're dealing with.
1740-
*
1741-
* We assume here that expression_planner() won't scribble on its input.
1742-
*/
1743-
expr=expression_planner(expr);
1744-
1745-
/* Now we can search for non-immutable functions */
1746-
returncontain_mutable_functions((Node*)expr);
1747-
}
1748-
1749-
17501723
/*
17511724
* CheckPredicate
17521725
*Checks that the given partial-index predicate is valid.
@@ -1770,7 +1743,7 @@ CheckPredicate(Expr *predicate)
17701743
* A predicate using mutable functions is probably wrong, for the same
17711744
* reasons that we don't allow an index expression to use one.
17721745
*/
1773-
if (CheckMutability(predicate))
1746+
if (contain_mutable_functions_after_planning(predicate))
17741747
ereport(ERROR,
17751748
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
17761749
errmsg("functions in index predicate must be marked IMMUTABLE")));
@@ -1913,7 +1886,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
19131886
* same data every time, it's not clear what the index entries
19141887
* mean at all.
19151888
*/
1916-
if (CheckMutability((Expr*)expr))
1889+
if (contain_mutable_functions_after_planning((Expr*)expr))
19171890
ereport(ERROR,
19181891
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
19191892
errmsg("functions in index expression must be marked IMMUTABLE")));

‎src/backend/commands/tablecmds.c

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16860,30 +16860,6 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1686016860
partattrs[attn] = 0;/* marks the column as expression */
1686116861
*partexprs = lappend(*partexprs, expr);
1686216862

16863-
/*
16864-
* Try to simplify the expression before checking for
16865-
* mutability. The main practical value of doing it in this
16866-
* order is that an inline-able SQL-language function will be
16867-
* accepted if its expansion is immutable, whether or not the
16868-
* function itself is marked immutable.
16869-
*
16870-
* Note that expression_planner does not change the passed in
16871-
* expression destructively and we have already saved the
16872-
* expression to be stored into the catalog above.
16873-
*/
16874-
expr = (Node *) expression_planner((Expr *) expr);
16875-
16876-
/*
16877-
* Partition expression cannot contain mutable functions,
16878-
* because a given row must always map to the same partition
16879-
* as long as there is no change in the partition boundary
16880-
* structure.
16881-
*/
16882-
if (contain_mutable_functions(expr))
16883-
ereport(ERROR,
16884-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
16885-
errmsg("functions in partition key expression must be marked IMMUTABLE")));
16886-
1688716863
/*
1688816864
* transformPartitionSpec() should have already rejected
1688916865
* subqueries, aggregates, window functions, and SRFs, based
@@ -16925,6 +16901,32 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1692516901
parser_errposition(pstate, pelem->location)));
1692616902
}
1692716903

16904+
/*
16905+
* Preprocess the expression before checking for mutability.
16906+
* This is essential for the reasons described in
16907+
* contain_mutable_functions_after_planning. However, we call
16908+
* expression_planner for ourselves rather than using that
16909+
* function, because if constant-folding reduces the
16910+
* expression to a constant, we'd like to know that so we can
16911+
* complain below.
16912+
*
16913+
* Like contain_mutable_functions_after_planning, assume that
16914+
* expression_planner won't scribble on its input, so this
16915+
* won't affect the partexprs entry we saved above.
16916+
*/
16917+
expr = (Node *) expression_planner((Expr *) expr);
16918+
16919+
/*
16920+
* Partition expressions cannot contain mutable functions,
16921+
* because a given row must always map to the same partition
16922+
* as long as there is no change in the partition boundary
16923+
* structure.
16924+
*/
16925+
if (contain_mutable_functions(expr))
16926+
ereport(ERROR,
16927+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
16928+
errmsg("functions in partition key expression must be marked IMMUTABLE")));
16929+
1692816930
/*
1692916931
* While it is not exactly *wrong* for a partition expression
1693016932
* to be a constant, it seems better to reject such keys.

‎src/backend/optimizer/util/clauses.c

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ contain_subplans_walker(Node *node, void *context)
357357
* mistakenly think that something like "WHERE random() < 0.5" can be treated
358358
* as a constant qualification.
359359
*
360+
* This will give the right answer only for clauses that have been put
361+
* through expression preprocessing. Callers outside the planner typically
362+
* should use contain_mutable_functions_after_planning() instead, for the
363+
* reasons given there.
364+
*
360365
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
361366
* but not into SubPlans. See comments for contain_volatile_functions().
362367
*/
@@ -416,6 +421,34 @@ contain_mutable_functions_walker(Node *node, void *context)
416421
context);
417422
}
418423

424+
/*
425+
* contain_mutable_functions_after_planning
426+
* Test whether given expression contains mutable functions.
427+
*
428+
* This is a wrapper for contain_mutable_functions() that is safe to use from
429+
* outside the planner. The difference is that it first runs the expression
430+
* through expression_planner(). There are two key reasons why we need that:
431+
*
432+
* First, function default arguments will get inserted, which may affect
433+
* volatility (consider "default now()").
434+
*
435+
* Second, inline-able functions will get inlined, which may allow us to
436+
* conclude that the function is really less volatile than it's marked.
437+
* As an example, polymorphic functions must be marked with the most volatile
438+
* behavior that they have for any input type, but once we inline the
439+
* function we may be able to conclude that it's not so volatile for the
440+
* particular input type we're dealing with.
441+
*/
442+
bool
443+
contain_mutable_functions_after_planning(Expr*expr)
444+
{
445+
/* We assume here that expression_planner() won't scribble on its input */
446+
expr=expression_planner(expr);
447+
448+
/* Now we can search for non-immutable functions */
449+
returncontain_mutable_functions((Node*)expr);
450+
}
451+
419452

420453
/*****************************************************************************
421454
*Check clauses for volatile functions
@@ -429,6 +462,11 @@ contain_mutable_functions_walker(Node *node, void *context)
429462
* volatile function) is found. This test prevents, for example,
430463
* invalid conversions of volatile expressions into indexscan quals.
431464
*
465+
* This will give the right answer only for clauses that have been put
466+
* through expression preprocessing. Callers outside the planner typically
467+
* should use contain_volatile_functions_after_planning() instead, for the
468+
* reasons given there.
469+
*
432470
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
433471
* but not into SubPlans. This is a bit odd, but intentional. If we are
434472
* looking at a SubLink, we are probably deciding whether a query tree
@@ -552,6 +590,34 @@ contain_volatile_functions_walker(Node *node, void *context)
552590
context);
553591
}
554592

593+
/*
594+
* contain_volatile_functions_after_planning
595+
* Test whether given expression contains volatile functions.
596+
*
597+
* This is a wrapper for contain_volatile_functions() that is safe to use from
598+
* outside the planner. The difference is that it first runs the expression
599+
* through expression_planner(). There are two key reasons why we need that:
600+
*
601+
* First, function default arguments will get inserted, which may affect
602+
* volatility (consider "default random()").
603+
*
604+
* Second, inline-able functions will get inlined, which may allow us to
605+
* conclude that the function is really less volatile than it's marked.
606+
* As an example, polymorphic functions must be marked with the most volatile
607+
* behavior that they have for any input type, but once we inline the
608+
* function we may be able to conclude that it's not so volatile for the
609+
* particular input type we're dealing with.
610+
*/
611+
bool
612+
contain_volatile_functions_after_planning(Expr*expr)
613+
{
614+
/* We assume here that expression_planner() won't scribble on its input */
615+
expr=expression_planner(expr);
616+
617+
/* Now we can search for volatile functions */
618+
returncontain_volatile_functions((Node*)expr);
619+
}
620+
555621
/*
556622
* Special purpose version of contain_volatile_functions() for use in COPY:
557623
* ignore nextval(), but treat all other functions normally.

‎src/include/optimizer/optimizer.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,9 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
136136
/* in util/clauses.c: */
137137

138138
externboolcontain_mutable_functions(Node*clause);
139+
externboolcontain_mutable_functions_after_planning(Expr*expr);
139140
externboolcontain_volatile_functions(Node*clause);
141+
externboolcontain_volatile_functions_after_planning(Expr*expr);
140142
externboolcontain_volatile_functions_not_nextval(Node*clause);
141143

142144
externNode*eval_const_expressions(PlannerInfo*root,Node*node);

‎src/test/regress/expected/fast_default.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,25 @@ SELECT comp();
272272
Rewritten
273273
(1 row)
274274

275+
-- check that we notice insertion of a volatile default argument
276+
CREATE FUNCTION foolme(timestamptz DEFAULT clock_timestamp())
277+
RETURNS timestamptz
278+
IMMUTABLE AS 'select $1' LANGUAGE sql;
279+
ALTER TABLE T ADD COLUMN c3 timestamptz DEFAULT foolme();
280+
NOTICE: rewriting table t for reason 2
281+
SELECT attname, atthasmissing, attmissingval FROM pg_attribute
282+
WHERE attrelid = 't'::regclass AND attnum > 0
283+
ORDER BY attnum;
284+
attname | atthasmissing | attmissingval
285+
---------+---------------+---------------
286+
pk | f |
287+
c1 | f |
288+
c2 | f |
289+
c3 | f |
290+
(4 rows)
291+
275292
DROP TABLE T;
293+
DROP FUNCTION foolme(timestamptz);
276294
-- Simple querie
277295
CREATE TABLE T (pk INT NOT NULL PRIMARY KEY);
278296
SELECT set('t');

‎src/test/regress/expected/generated.out

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ LINE 1: ..._3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (c * 2) STO...
6161
-- generation expression must be immutable
6262
CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED ALWAYS AS (random()) STORED);
6363
ERROR: generation expression is not immutable
64+
-- ... but be sure that the immutability test is accurate
65+
CREATE TABLE gtest2 (a int, b text GENERATED ALWAYS AS (a || ' sec') STORED);
66+
DROP TABLE gtest2;
6467
-- cannot have default/identity and generated
6568
CREATE TABLE gtest_err_5a (a int PRIMARY KEY, b int DEFAULT 5 GENERATED ALWAYS AS (a * 2) STORED);
6669
ERROR: both default and generation expression specified for column "b" of table "gtest_err_5a"

‎src/test/regress/sql/fast_default.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,18 @@ ALTER TABLE T ADD COLUMN c2 TIMESTAMP DEFAULT clock_timestamp();
256256

257257
SELECT comp();
258258

259+
-- check that we notice insertion of a volatile default argument
260+
CREATEFUNCTIONfoolme(timestamptz DEFAULT clock_timestamp())
261+
RETURNStimestamptz
262+
IMMUTABLEAS'select $1' LANGUAGE sql;
263+
ALTERTABLE T ADD COLUMN c3timestamptz DEFAULT foolme();
264+
265+
SELECT attname, atthasmissing, attmissingvalFROM pg_attribute
266+
WHERE attrelid='t'::regclassAND attnum>0
267+
ORDER BY attnum;
268+
259269
DROPTABLE T;
270+
DROPFUNCTION foolme(timestamptz);
260271

261272
-- Simple querie
262273
CREATETABLET (pkINTNOT NULLPRIMARY KEY);

‎src/test/regress/sql/generated.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ CREATE TABLE gtest_err_3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (c * 2) S
2626

2727
-- generation expression must be immutable
2828
CREATETABLEgtest_err_4 (aintPRIMARY KEY, bdouble precision GENERATED ALWAYSAS (random()) STORED);
29+
-- ... but be sure that the immutability test is accurate
30+
CREATETABLEgtest2 (aint, btext GENERATED ALWAYSAS (a||' sec') STORED);
31+
DROPTABLE gtest2;
2932

3033
-- cannot have default/identity and generated
3134
CREATETABLEgtest_err_5a (aintPRIMARY KEY, bint DEFAULT5 GENERATED ALWAYSAS (a*2) STORED);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp