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

Commit5c11104

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 parent231a445 commit5c11104

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

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

26182619
defOid=StoreAttrDefault(rel,colDef->attnum,expr,is_internal,
@@ -3047,9 +3048,11 @@ cookDefault(ParseState *pstate,
30473048

30483049
if (attgenerated)
30493050
{
3051+
/* Disallow refs to other generated columns */
30503052
check_nested_generated(pstate,expr);
30513053

3052-
if (contain_mutable_functions(expr))
3054+
/* Disallow mutable functions */
3055+
if (contain_mutable_functions_after_planning((Expr*)expr))
30533056
ereport(ERROR,
30543057
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
30553058
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
@@ -2893,6 +2893,9 @@ CopyFrom(CopyState cstate)
28932893
* Can't support multi-inserts if there are any volatile function
28942894
* expressions in WHERE clause. Similarly to the trigger case above,
28952895
* such expressions may query the table we're inserting into.
2896+
*
2897+
* Note: the whereClause was already preprocessed in DoCopy(), so it's
2898+
* okay to use contain_volatile_functions() directly.
28962899
*/
28972900
insertMethod=CIM_SINGLE;
28982901
}
@@ -3441,7 +3444,8 @@ BeginCopyFrom(ParseState *pstate,
34413444
* known to be safe for use with the multi-insert
34423445
* optimization. Hence we use this special case function
34433446
* checker rather than the standard check for
3444-
* contain_volatile_functions().
3447+
* contain_volatile_functions(). Note also that we already
3448+
* ran the expression through expression_planner().
34453449
*/
34463450
if (!volatile_defexprs)
34473451
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
@@ -1667,33 +1667,6 @@ DefineIndex(Oid relationId,
16671667
}
16681668

16691669

1670-
/*
1671-
* CheckMutability
1672-
*Test whether given expression is mutable
1673-
*/
1674-
staticbool
1675-
CheckMutability(Expr*expr)
1676-
{
1677-
/*
1678-
* First run the expression through the planner. This has a couple of
1679-
* important consequences. First, function default arguments will get
1680-
* inserted, which may affect volatility (consider "default now()").
1681-
* Second, inline-able functions will get inlined, which may allow us to
1682-
* conclude that the function is really less volatile than it's marked. As
1683-
* an example, polymorphic functions must be marked with the most volatile
1684-
* behavior that they have for any input type, but once we inline the
1685-
* function we may be able to conclude that it's not so volatile for the
1686-
* particular input type we're dealing with.
1687-
*
1688-
* We assume here that expression_planner() won't scribble on its input.
1689-
*/
1690-
expr=expression_planner(expr);
1691-
1692-
/* Now we can search for non-immutable functions */
1693-
returncontain_mutable_functions((Node*)expr);
1694-
}
1695-
1696-
16971670
/*
16981671
* CheckPredicate
16991672
*Checks that the given partial-index predicate is valid.
@@ -1717,7 +1690,7 @@ CheckPredicate(Expr *predicate)
17171690
* A predicate using mutable functions is probably wrong, for the same
17181691
* reasons that we don't allow an index expression to use one.
17191692
*/
1720-
if (CheckMutability(predicate))
1693+
if (contain_mutable_functions_after_planning(predicate))
17211694
ereport(ERROR,
17221695
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
17231696
errmsg("functions in index predicate must be marked IMMUTABLE")));
@@ -1860,7 +1833,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
18601833
* same data every time, it's not clear what the index entries
18611834
* mean at all.
18621835
*/
1863-
if (CheckMutability((Expr*)expr))
1836+
if (contain_mutable_functions_after_planning((Expr*)expr))
18641837
ereport(ERROR,
18651838
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
18661839
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
@@ -16355,30 +16355,6 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1635516355
partattrs[attn] = 0;/* marks the column as expression */
1635616356
*partexprs = lappend(*partexprs, expr);
1635716357

16358-
/*
16359-
* Try to simplify the expression before checking for
16360-
* mutability. The main practical value of doing it in this
16361-
* order is that an inline-able SQL-language function will be
16362-
* accepted if its expansion is immutable, whether or not the
16363-
* function itself is marked immutable.
16364-
*
16365-
* Note that expression_planner does not change the passed in
16366-
* expression destructively and we have already saved the
16367-
* expression to be stored into the catalog above.
16368-
*/
16369-
expr = (Node *) expression_planner((Expr *) expr);
16370-
16371-
/*
16372-
* Partition expression cannot contain mutable functions,
16373-
* because a given row must always map to the same partition
16374-
* as long as there is no change in the partition boundary
16375-
* structure.
16376-
*/
16377-
if (contain_mutable_functions(expr))
16378-
ereport(ERROR,
16379-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
16380-
errmsg("functions in partition key expression must be marked IMMUTABLE")));
16381-
1638216358
/*
1638316359
* transformPartitionSpec() should have already rejected
1638416360
* subqueries, aggregates, window functions, and SRFs, based
@@ -16420,6 +16396,32 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1642016396
parser_errposition(pstate, pelem->location)));
1642116397
}
1642216398

16399+
/*
16400+
* Preprocess the expression before checking for mutability.
16401+
* This is essential for the reasons described in
16402+
* contain_mutable_functions_after_planning. However, we call
16403+
* expression_planner for ourselves rather than using that
16404+
* function, because if constant-folding reduces the
16405+
* expression to a constant, we'd like to know that so we can
16406+
* complain below.
16407+
*
16408+
* Like contain_mutable_functions_after_planning, assume that
16409+
* expression_planner won't scribble on its input, so this
16410+
* won't affect the partexprs entry we saved above.
16411+
*/
16412+
expr = (Node *) expression_planner((Expr *) expr);
16413+
16414+
/*
16415+
* Partition expressions cannot contain mutable functions,
16416+
* because a given row must always map to the same partition
16417+
* as long as there is no change in the partition boundary
16418+
* structure.
16419+
*/
16420+
if (contain_mutable_functions(expr))
16421+
ereport(ERROR,
16422+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
16423+
errmsg("functions in partition key expression must be marked IMMUTABLE")));
16424+
1642316425
/*
1642416426
* While it is not exactly *wrong* for a partition expression
1642516427
* 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
@@ -641,6 +641,11 @@ contain_subplans_walker(Node *node, void *context)
641641
* mistakenly think that something like "WHERE random() < 0.5" can be treated
642642
* as a constant qualification.
643643
*
644+
* This will give the right answer only for clauses that have been put
645+
* through expression preprocessing. Callers outside the planner typically
646+
* should use contain_mutable_functions_after_planning() instead, for the
647+
* reasons given there.
648+
*
644649
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
645650
* but not into SubPlans. See comments for contain_volatile_functions().
646651
*/
@@ -700,6 +705,34 @@ contain_mutable_functions_walker(Node *node, void *context)
700705
context);
701706
}
702707

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

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

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

133133
externboolcontain_mutable_functions(Node*clause);
134+
externboolcontain_mutable_functions_after_planning(Expr*expr);
134135
externboolcontain_volatile_functions(Node*clause);
136+
externboolcontain_volatile_functions_after_planning(Expr*expr);
135137
externboolcontain_volatile_functions_not_nextval(Node*clause);
136138

137139
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