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

Commitabd1b13

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 parentc236778 commitabd1b13

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
@@ -2613,7 +2613,8 @@ AddRelationNewConstraints(Relation rel,
26132613
continue;
26142614

26152615
/* If the DEFAULT is volatile we cannot use a missing value */
2616-
if (colDef->missingMode&&contain_volatile_functions((Node*)expr))
2616+
if (colDef->missingMode&&
2617+
contain_volatile_functions_after_planning((Expr*)expr))
26172618
colDef->missingMode= false;
26182619

26192620
defOid=StoreAttrDefault(rel,colDef->attnum,expr,is_internal,
@@ -3048,9 +3049,11 @@ cookDefault(ParseState *pstate,
30483049

30493050
if (attgenerated)
30503051
{
3052+
/* Disallow refs to other generated columns */
30513053
check_nested_generated(pstate,expr);
30523054

3053-
if (contain_mutable_functions(expr))
3055+
/* Disallow mutable functions */
3056+
if (contain_mutable_functions_after_planning((Expr*)expr))
30543057
ereport(ERROR,
30553058
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
30563059
errmsg("generation expression is not immutable")));

‎src/backend/commands/copy.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2947,6 +2947,9 @@ CopyFrom(CopyState cstate)
29472947
* Can't support multi-inserts if there are any volatile function
29482948
* expressions in WHERE clause. Similarly to the trigger case above,
29492949
* such expressions may query the table we're inserting into.
2950+
*
2951+
* Note: the whereClause was already preprocessed in DoCopy(), so it's
2952+
* okay to use contain_volatile_functions() directly.
29502953
*/
29512954
insertMethod=CIM_SINGLE;
29522955
}
@@ -3495,7 +3498,8 @@ BeginCopyFrom(ParseState *pstate,
34953498
* known to be safe for use with the multi-insert
34963499
* optimization. Hence we use this special case function
34973500
* checker rather than the standard check for
3498-
* contain_volatile_functions().
3501+
* contain_volatile_functions(). Note also that we already
3502+
* ran the expression through expression_planner().
34993503
*/
35003504
if (!volatile_defexprs)
35013505
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
@@ -1619,33 +1619,6 @@ DefineIndex(Oid relationId,
16191619
}
16201620

16211621

1622-
/*
1623-
* CheckMutability
1624-
*Test whether given expression is mutable
1625-
*/
1626-
staticbool
1627-
CheckMutability(Expr*expr)
1628-
{
1629-
/*
1630-
* First run the expression through the planner. This has a couple of
1631-
* important consequences. First, function default arguments will get
1632-
* inserted, which may affect volatility (consider "default now()").
1633-
* Second, inline-able functions will get inlined, which may allow us to
1634-
* conclude that the function is really less volatile than it's marked. As
1635-
* an example, polymorphic functions must be marked with the most volatile
1636-
* behavior that they have for any input type, but once we inline the
1637-
* function we may be able to conclude that it's not so volatile for the
1638-
* particular input type we're dealing with.
1639-
*
1640-
* We assume here that expression_planner() won't scribble on its input.
1641-
*/
1642-
expr=expression_planner(expr);
1643-
1644-
/* Now we can search for non-immutable functions */
1645-
returncontain_mutable_functions((Node*)expr);
1646-
}
1647-
1648-
16491622
/*
16501623
* CheckPredicate
16511624
*Checks that the given partial-index predicate is valid.
@@ -1669,7 +1642,7 @@ CheckPredicate(Expr *predicate)
16691642
* A predicate using mutable functions is probably wrong, for the same
16701643
* reasons that we don't allow an index expression to use one.
16711644
*/
1672-
if (CheckMutability(predicate))
1645+
if (contain_mutable_functions_after_planning(predicate))
16731646
ereport(ERROR,
16741647
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
16751648
errmsg("functions in index predicate must be marked IMMUTABLE")));
@@ -1812,7 +1785,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
18121785
* same data every time, it's not clear what the index entries
18131786
* mean at all.
18141787
*/
1815-
if (CheckMutability((Expr*)expr))
1788+
if (contain_mutable_functions_after_planning((Expr*)expr))
18161789
ereport(ERROR,
18171790
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
18181791
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
@@ -15888,30 +15888,6 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1588815888
partattrs[attn] = 0;/* marks the column as expression */
1588915889
*partexprs = lappend(*partexprs, expr);
1589015890

15891-
/*
15892-
* Try to simplify the expression before checking for
15893-
* mutability. The main practical value of doing it in this
15894-
* order is that an inline-able SQL-language function will be
15895-
* accepted if its expansion is immutable, whether or not the
15896-
* function itself is marked immutable.
15897-
*
15898-
* Note that expression_planner does not change the passed in
15899-
* expression destructively and we have already saved the
15900-
* expression to be stored into the catalog above.
15901-
*/
15902-
expr = (Node *) expression_planner((Expr *) expr);
15903-
15904-
/*
15905-
* Partition expression cannot contain mutable functions,
15906-
* because a given row must always map to the same partition
15907-
* as long as there is no change in the partition boundary
15908-
* structure.
15909-
*/
15910-
if (contain_mutable_functions(expr))
15911-
ereport(ERROR,
15912-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
15913-
errmsg("functions in partition key expression must be marked IMMUTABLE")));
15914-
1591515891
/*
1591615892
* transformPartitionSpec() should have already rejected
1591715893
* subqueries, aggregates, window functions, and SRFs, based
@@ -15956,6 +15932,32 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1595615932
parser_errposition(pstate, pelem->location)));
1595715933
}
1595815934

15935+
/*
15936+
* Preprocess the expression before checking for mutability.
15937+
* This is essential for the reasons described in
15938+
* contain_mutable_functions_after_planning. However, we call
15939+
* expression_planner for ourselves rather than using that
15940+
* function, because if constant-folding reduces the
15941+
* expression to a constant, we'd like to know that so we can
15942+
* complain below.
15943+
*
15944+
* Like contain_mutable_functions_after_planning, assume that
15945+
* expression_planner won't scribble on its input, so this
15946+
* won't affect the partexprs entry we saved above.
15947+
*/
15948+
expr = (Node *) expression_planner((Expr *) expr);
15949+
15950+
/*
15951+
* Partition expressions cannot contain mutable functions,
15952+
* because a given row must always map to the same partition
15953+
* as long as there is no change in the partition boundary
15954+
* structure.
15955+
*/
15956+
if (contain_mutable_functions(expr))
15957+
ereport(ERROR,
15958+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
15959+
errmsg("functions in partition key expression must be marked IMMUTABLE")));
15960+
1595915961
/*
1596015962
* While it is not exactly *wrong* for a partition expression
1596115963
* 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
@@ -642,6 +642,11 @@ contain_subplans_walker(Node *node, void *context)
642642
* mistakenly think that something like "WHERE random() < 0.5" can be treated
643643
* as a constant qualification.
644644
*
645+
* This will give the right answer only for clauses that have been put
646+
* through expression preprocessing. Callers outside the planner typically
647+
* should use contain_mutable_functions_after_planning() instead, for the
648+
* reasons given there.
649+
*
645650
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
646651
* but not into SubPlans. See comments for contain_volatile_functions().
647652
*/
@@ -701,6 +706,34 @@ contain_mutable_functions_walker(Node *node, void *context)
701706
context);
702707
}
703708

709+
/*
710+
* contain_mutable_functions_after_planning
711+
* Test whether given expression contains mutable functions.
712+
*
713+
* This is a wrapper for contain_mutable_functions() that is safe to use from
714+
* outside the planner. The difference is that it first runs the expression
715+
* through expression_planner(). There are two key reasons why we need that:
716+
*
717+
* First, function default arguments will get inserted, which may affect
718+
* volatility (consider "default now()").
719+
*
720+
* Second, inline-able functions will get inlined, which may allow us to
721+
* conclude that the function is really less volatile than it's marked.
722+
* As an example, polymorphic functions must be marked with the most volatile
723+
* behavior that they have for any input type, but once we inline the
724+
* function we may be able to conclude that it's not so volatile for the
725+
* particular input type we're dealing with.
726+
*/
727+
bool
728+
contain_mutable_functions_after_planning(Expr*expr)
729+
{
730+
/* We assume here that expression_planner() won't scribble on its input */
731+
expr=expression_planner(expr);
732+
733+
/* Now we can search for non-immutable functions */
734+
returncontain_mutable_functions((Node*)expr);
735+
}
736+
704737

705738
/*****************************************************************************
706739
*Check clauses for volatile functions
@@ -714,6 +747,11 @@ contain_mutable_functions_walker(Node *node, void *context)
714747
* volatile function) is found. This test prevents, for example,
715748
* invalid conversions of volatile expressions into indexscan quals.
716749
*
750+
* This will give the right answer only for clauses that have been put
751+
* through expression preprocessing. Callers outside the planner typically
752+
* should use contain_volatile_functions_after_planning() instead, for the
753+
* reasons given there.
754+
*
717755
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
718756
* but not into SubPlans. This is a bit odd, but intentional. If we are
719757
* looking at a SubLink, we are probably deciding whether a query tree
@@ -770,6 +808,34 @@ contain_volatile_functions_walker(Node *node, void *context)
770808
context);
771809
}
772810

811+
/*
812+
* contain_volatile_functions_after_planning
813+
* Test whether given expression contains volatile functions.
814+
*
815+
* This is a wrapper for contain_volatile_functions() that is safe to use from
816+
* outside the planner. The difference is that it first runs the expression
817+
* through expression_planner(). There are two key reasons why we need that:
818+
*
819+
* First, function default arguments will get inserted, which may affect
820+
* volatility (consider "default random()").
821+
*
822+
* Second, inline-able functions will get inlined, which may allow us to
823+
* conclude that the function is really less volatile than it's marked.
824+
* As an example, polymorphic functions must be marked with the most volatile
825+
* behavior that they have for any input type, but once we inline the
826+
* function we may be able to conclude that it's not so volatile for the
827+
* particular input type we're dealing with.
828+
*/
829+
bool
830+
contain_volatile_functions_after_planning(Expr*expr)
831+
{
832+
/* We assume here that expression_planner() won't scribble on its input */
833+
expr=expression_planner(expr);
834+
835+
/* Now we can search for volatile functions */
836+
returncontain_volatile_functions((Node*)expr);
837+
}
838+
773839
/*
774840
* Special purpose version of contain_volatile_functions() for use in COPY:
775841
* 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
@@ -130,7 +130,9 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
130130
/* in util/clauses.c: */
131131

132132
externboolcontain_mutable_functions(Node*clause);
133+
externboolcontain_mutable_functions_after_planning(Expr*expr);
133134
externboolcontain_volatile_functions(Node*clause);
135+
externboolcontain_volatile_functions_after_planning(Expr*expr);
134136
externboolcontain_volatile_functions_not_nextval(Node*clause);
135137

136138
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