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

Commit9057ddb

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 parent18f4798 commit9057ddb

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
@@ -2328,7 +2328,8 @@ AddRelationNewConstraints(Relation rel,
23282328
continue;
23292329

23302330
/* If the DEFAULT is volatile we cannot use a missing value */
2331-
if (colDef->missingMode&&contain_volatile_functions((Node*)expr))
2331+
if (colDef->missingMode&&
2332+
contain_volatile_functions_after_planning((Expr*)expr))
23322333
colDef->missingMode= false;
23332334

23342335
defOid=StoreAttrDefault(rel,colDef->attnum,expr,is_internal,
@@ -2763,9 +2764,11 @@ cookDefault(ParseState *pstate,
27632764

27642765
if (attgenerated)
27652766
{
2767+
/* Disallow refs to other generated columns */
27662768
check_nested_generated(pstate,expr);
27672769

2768-
if (contain_mutable_functions(expr))
2770+
/* Disallow mutable functions */
2771+
if (contain_mutable_functions_after_planning((Expr*)expr))
27692772
ereport(ERROR,
27702773
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
27712774
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
@@ -758,6 +758,9 @@ CopyFrom(CopyFromState cstate)
758758
* Can't support multi-inserts if there are any volatile function
759759
* expressions in WHERE clause. Similarly to the trigger case above,
760760
* such expressions may query the table we're inserting into.
761+
*
762+
* Note: the whereClause was already preprocessed in DoCopy(), so it's
763+
* okay to use contain_volatile_functions() directly.
761764
*/
762765
insertMethod=CIM_SINGLE;
763766
}
@@ -1453,7 +1456,8 @@ BeginCopyFrom(ParseState *pstate,
14531456
* known to be safe for use with the multi-insert
14541457
* optimization. Hence we use this special case function
14551458
* checker rather than the standard check for
1456-
* contain_volatile_functions().
1459+
* contain_volatile_functions(). Note also that we already
1460+
* ran the expression through expression_planner().
14571461
*/
14581462
if (!volatile_defexprs)
14591463
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
@@ -1711,33 +1711,6 @@ DefineIndex(Oid relationId,
17111711
}
17121712

17131713

1714-
/*
1715-
* CheckMutability
1716-
*Test whether given expression is mutable
1717-
*/
1718-
staticbool
1719-
CheckMutability(Expr*expr)
1720-
{
1721-
/*
1722-
* First run the expression through the planner. This has a couple of
1723-
* important consequences. First, function default arguments will get
1724-
* inserted, which may affect volatility (consider "default now()").
1725-
* Second, inline-able functions will get inlined, which may allow us to
1726-
* conclude that the function is really less volatile than it's marked. As
1727-
* an example, polymorphic functions must be marked with the most volatile
1728-
* behavior that they have for any input type, but once we inline the
1729-
* function we may be able to conclude that it's not so volatile for the
1730-
* particular input type we're dealing with.
1731-
*
1732-
* We assume here that expression_planner() won't scribble on its input.
1733-
*/
1734-
expr=expression_planner(expr);
1735-
1736-
/* Now we can search for non-immutable functions */
1737-
returncontain_mutable_functions((Node*)expr);
1738-
}
1739-
1740-
17411714
/*
17421715
* CheckPredicate
17431716
*Checks that the given partial-index predicate is valid.
@@ -1761,7 +1734,7 @@ CheckPredicate(Expr *predicate)
17611734
* A predicate using mutable functions is probably wrong, for the same
17621735
* reasons that we don't allow an index expression to use one.
17631736
*/
1764-
if (CheckMutability(predicate))
1737+
if (contain_mutable_functions_after_planning(predicate))
17651738
ereport(ERROR,
17661739
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
17671740
errmsg("functions in index predicate must be marked IMMUTABLE")));
@@ -1904,7 +1877,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
19041877
* same data every time, it's not clear what the index entries
19051878
* mean at all.
19061879
*/
1907-
if (CheckMutability((Expr*)expr))
1880+
if (contain_mutable_functions_after_planning((Expr*)expr))
19081881
ereport(ERROR,
19091882
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
19101883
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
@@ -17406,30 +17406,6 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1740617406
partattrs[attn] = 0;/* marks the column as expression */
1740717407
*partexprs = lappend(*partexprs, expr);
1740817408

17409-
/*
17410-
* Try to simplify the expression before checking for
17411-
* mutability. The main practical value of doing it in this
17412-
* order is that an inline-able SQL-language function will be
17413-
* accepted if its expansion is immutable, whether or not the
17414-
* function itself is marked immutable.
17415-
*
17416-
* Note that expression_planner does not change the passed in
17417-
* expression destructively and we have already saved the
17418-
* expression to be stored into the catalog above.
17419-
*/
17420-
expr = (Node *) expression_planner((Expr *) expr);
17421-
17422-
/*
17423-
* Partition expression cannot contain mutable functions,
17424-
* because a given row must always map to the same partition
17425-
* as long as there is no change in the partition boundary
17426-
* structure.
17427-
*/
17428-
if (contain_mutable_functions(expr))
17429-
ereport(ERROR,
17430-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
17431-
errmsg("functions in partition key expression must be marked IMMUTABLE")));
17432-
1743317409
/*
1743417410
* transformPartitionSpec() should have already rejected
1743517411
* subqueries, aggregates, window functions, and SRFs, based
@@ -17471,6 +17447,32 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1747117447
parser_errposition(pstate, pelem->location)));
1747217448
}
1747317449

17450+
/*
17451+
* Preprocess the expression before checking for mutability.
17452+
* This is essential for the reasons described in
17453+
* contain_mutable_functions_after_planning. However, we call
17454+
* expression_planner for ourselves rather than using that
17455+
* function, because if constant-folding reduces the
17456+
* expression to a constant, we'd like to know that so we can
17457+
* complain below.
17458+
*
17459+
* Like contain_mutable_functions_after_planning, assume that
17460+
* expression_planner won't scribble on its input, so this
17461+
* won't affect the partexprs entry we saved above.
17462+
*/
17463+
expr = (Node *) expression_planner((Expr *) expr);
17464+
17465+
/*
17466+
* Partition expressions cannot contain mutable functions,
17467+
* because a given row must always map to the same partition
17468+
* as long as there is no change in the partition boundary
17469+
* structure.
17470+
*/
17471+
if (contain_mutable_functions(expr))
17472+
ereport(ERROR,
17473+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
17474+
errmsg("functions in partition key expression must be marked IMMUTABLE")));
17475+
1747417476
/*
1747517477
* While it is not exactly *wrong* for a partition expression
1747617478
* 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
@@ -138,7 +138,9 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
138138
/* in util/clauses.c: */
139139

140140
externboolcontain_mutable_functions(Node*clause);
141+
externboolcontain_mutable_functions_after_planning(Expr*expr);
141142
externboolcontain_volatile_functions(Node*clause);
143+
externboolcontain_volatile_functions_after_planning(Expr*expr);
142144
externboolcontain_volatile_functions_not_nextval(Node*clause);
143145

144146
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