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

Commit743ddaf

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 parentbc15a12 commit743ddaf

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
@@ -2365,7 +2365,8 @@ AddRelationNewConstraints(Relation rel,
23652365
continue;
23662366

23672367
/* If the DEFAULT is volatile we cannot use a missing value */
2368-
if (colDef->missingMode&&contain_volatile_functions((Node*)expr))
2368+
if (colDef->missingMode&&
2369+
contain_volatile_functions_after_planning((Expr*)expr))
23692370
colDef->missingMode= false;
23702371

23712372
defOid=StoreAttrDefault(rel,colDef->attnum,expr,is_internal,
@@ -3087,9 +3088,11 @@ cookDefault(ParseState *pstate,
30873088

30883089
if (attgenerated)
30893090
{
3091+
/* Disallow refs to other generated columns */
30903092
check_nested_generated(pstate,expr);
30913093

3092-
if (contain_mutable_functions(expr))
3094+
/* Disallow mutable functions */
3095+
if (contain_mutable_functions_after_planning((Expr*)expr))
30933096
ereport(ERROR,
30943097
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
30953098
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
@@ -887,6 +887,9 @@ CopyFrom(CopyFromState cstate)
887887
* Can't support multi-inserts if there are any volatile function
888888
* expressions in WHERE clause. Similarly to the trigger case above,
889889
* such expressions may query the table we're inserting into.
890+
*
891+
* Note: the whereClause was already preprocessed in DoCopy(), so it's
892+
* okay to use contain_volatile_functions() directly.
890893
*/
891894
insertMethod=CIM_SINGLE;
892895
}
@@ -1616,7 +1619,8 @@ BeginCopyFrom(ParseState *pstate,
16161619
* known to be safe for use with the multi-insert
16171620
* optimization. Hence we use this special case function
16181621
* checker rather than the standard check for
1619-
* contain_volatile_functions().
1622+
* contain_volatile_functions(). Note also that we already
1623+
* ran the expression through expression_planner().
16201624
*/
16211625
if (!volatile_defexprs)
16221626
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
@@ -1791,33 +1791,6 @@ DefineIndex(Oid tableId,
17911791
}
17921792

17931793

1794-
/*
1795-
* CheckMutability
1796-
*Test whether given expression is mutable
1797-
*/
1798-
staticbool
1799-
CheckMutability(Expr*expr)
1800-
{
1801-
/*
1802-
* First run the expression through the planner. This has a couple of
1803-
* important consequences. First, function default arguments will get
1804-
* inserted, which may affect volatility (consider "default now()").
1805-
* Second, inline-able functions will get inlined, which may allow us to
1806-
* conclude that the function is really less volatile than it's marked. As
1807-
* an example, polymorphic functions must be marked with the most volatile
1808-
* behavior that they have for any input type, but once we inline the
1809-
* function we may be able to conclude that it's not so volatile for the
1810-
* particular input type we're dealing with.
1811-
*
1812-
* We assume here that expression_planner() won't scribble on its input.
1813-
*/
1814-
expr=expression_planner(expr);
1815-
1816-
/* Now we can search for non-immutable functions */
1817-
returncontain_mutable_functions((Node*)expr);
1818-
}
1819-
1820-
18211794
/*
18221795
* CheckPredicate
18231796
*Checks that the given partial-index predicate is valid.
@@ -1841,7 +1814,7 @@ CheckPredicate(Expr *predicate)
18411814
* A predicate using mutable functions is probably wrong, for the same
18421815
* reasons that we don't allow an index expression to use one.
18431816
*/
1844-
if (CheckMutability(predicate))
1817+
if (contain_mutable_functions_after_planning(predicate))
18451818
ereport(ERROR,
18461819
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
18471820
errmsg("functions in index predicate must be marked IMMUTABLE")));
@@ -1985,7 +1958,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
19851958
* same data every time, it's not clear what the index entries
19861959
* mean at all.
19871960
*/
1988-
if (CheckMutability((Expr*)expr))
1961+
if (contain_mutable_functions_after_planning((Expr*)expr))
19891962
ereport(ERROR,
19901963
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
19911964
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
@@ -18159,30 +18159,6 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1815918159
partattrs[attn] = 0;/* marks the column as expression */
1816018160
*partexprs = lappend(*partexprs, expr);
1816118161

18162-
/*
18163-
* Try to simplify the expression before checking for
18164-
* mutability. The main practical value of doing it in this
18165-
* order is that an inline-able SQL-language function will be
18166-
* accepted if its expansion is immutable, whether or not the
18167-
* function itself is marked immutable.
18168-
*
18169-
* Note that expression_planner does not change the passed in
18170-
* expression destructively and we have already saved the
18171-
* expression to be stored into the catalog above.
18172-
*/
18173-
expr = (Node *) expression_planner((Expr *) expr);
18174-
18175-
/*
18176-
* Partition expression cannot contain mutable functions,
18177-
* because a given row must always map to the same partition
18178-
* as long as there is no change in the partition boundary
18179-
* structure.
18180-
*/
18181-
if (contain_mutable_functions(expr))
18182-
ereport(ERROR,
18183-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
18184-
errmsg("functions in partition key expression must be marked IMMUTABLE")));
18185-
1818618162
/*
1818718163
* transformPartitionSpec() should have already rejected
1818818164
* subqueries, aggregates, window functions, and SRFs, based
@@ -18224,6 +18200,32 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
1822418200
parser_errposition(pstate, pelem->location)));
1822518201
}
1822618202

18203+
/*
18204+
* Preprocess the expression before checking for mutability.
18205+
* This is essential for the reasons described in
18206+
* contain_mutable_functions_after_planning. However, we call
18207+
* expression_planner for ourselves rather than using that
18208+
* function, because if constant-folding reduces the
18209+
* expression to a constant, we'd like to know that so we can
18210+
* complain below.
18211+
*
18212+
* Like contain_mutable_functions_after_planning, assume that
18213+
* expression_planner won't scribble on its input, so this
18214+
* won't affect the partexprs entry we saved above.
18215+
*/
18216+
expr = (Node *) expression_planner((Expr *) expr);
18217+
18218+
/*
18219+
* Partition expressions cannot contain mutable functions,
18220+
* because a given row must always map to the same partition
18221+
* as long as there is no change in the partition boundary
18222+
* structure.
18223+
*/
18224+
if (contain_mutable_functions(expr))
18225+
ereport(ERROR,
18226+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
18227+
errmsg("functions in partition key expression must be marked IMMUTABLE")));
18228+
1822718229
/*
1822818230
* While it is not exactly *wrong* for a partition expression
1822918231
* 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
@@ -360,6 +360,11 @@ contain_subplans_walker(Node *node, void *context)
360360
* mistakenly think that something like "WHERE random() < 0.5" can be treated
361361
* as a constant qualification.
362362
*
363+
* This will give the right answer only for clauses that have been put
364+
* through expression preprocessing. Callers outside the planner typically
365+
* should use contain_mutable_functions_after_planning() instead, for the
366+
* reasons given there.
367+
*
363368
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
364369
* but not into SubPlans. See comments for contain_volatile_functions().
365370
*/
@@ -446,6 +451,34 @@ contain_mutable_functions_walker(Node *node, void *context)
446451
context);
447452
}
448453

454+
/*
455+
* contain_mutable_functions_after_planning
456+
* Test whether given expression contains mutable functions.
457+
*
458+
* This is a wrapper for contain_mutable_functions() that is safe to use from
459+
* outside the planner. The difference is that it first runs the expression
460+
* through expression_planner(). There are two key reasons why we need that:
461+
*
462+
* First, function default arguments will get inserted, which may affect
463+
* volatility (consider "default now()").
464+
*
465+
* Second, inline-able functions will get inlined, which may allow us to
466+
* conclude that the function is really less volatile than it's marked.
467+
* As an example, polymorphic functions must be marked with the most volatile
468+
* behavior that they have for any input type, but once we inline the
469+
* function we may be able to conclude that it's not so volatile for the
470+
* particular input type we're dealing with.
471+
*/
472+
bool
473+
contain_mutable_functions_after_planning(Expr*expr)
474+
{
475+
/* We assume here that expression_planner() won't scribble on its input */
476+
expr=expression_planner(expr);
477+
478+
/* Now we can search for non-immutable functions */
479+
returncontain_mutable_functions((Node*)expr);
480+
}
481+
449482

450483
/*****************************************************************************
451484
*Check clauses for volatile functions
@@ -459,6 +492,11 @@ contain_mutable_functions_walker(Node *node, void *context)
459492
* volatile function) is found. This test prevents, for example,
460493
* invalid conversions of volatile expressions into indexscan quals.
461494
*
495+
* This will give the right answer only for clauses that have been put
496+
* through expression preprocessing. Callers outside the planner typically
497+
* should use contain_volatile_functions_after_planning() instead, for the
498+
* reasons given there.
499+
*
462500
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
463501
* but not into SubPlans. This is a bit odd, but intentional. If we are
464502
* looking at a SubLink, we are probably deciding whether a query tree
@@ -582,6 +620,34 @@ contain_volatile_functions_walker(Node *node, void *context)
582620
context);
583621
}
584622

623+
/*
624+
* contain_volatile_functions_after_planning
625+
* Test whether given expression contains volatile functions.
626+
*
627+
* This is a wrapper for contain_volatile_functions() that is safe to use from
628+
* outside the planner. The difference is that it first runs the expression
629+
* through expression_planner(). There are two key reasons why we need that:
630+
*
631+
* First, function default arguments will get inserted, which may affect
632+
* volatility (consider "default random()").
633+
*
634+
* Second, inline-able functions will get inlined, which may allow us to
635+
* conclude that the function is really less volatile than it's marked.
636+
* As an example, polymorphic functions must be marked with the most volatile
637+
* behavior that they have for any input type, but once we inline the
638+
* function we may be able to conclude that it's not so volatile for the
639+
* particular input type we're dealing with.
640+
*/
641+
bool
642+
contain_volatile_functions_after_planning(Expr*expr)
643+
{
644+
/* We assume here that expression_planner() won't scribble on its input */
645+
expr=expression_planner(expr);
646+
647+
/* Now we can search for volatile functions */
648+
returncontain_volatile_functions((Node*)expr);
649+
}
650+
585651
/*
586652
* Special purpose version of contain_volatile_functions() for use in COPY:
587653
* 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