Subject: Re: BUG #18097: Immutable expression not allowed in generated at

From: "David G. Johnston" <[email-masked]>
Date: Fri, 8 Sep 2023 08:11:42 -0700

On Thursday, September 7, 2023, PG Bug reporting form <
[email-masked]> wrote:

> The following bug has been logged on the website:
>
> Bug reference:      18097
> Logged by:          Jim Keener
> Email address:      [email-masked]
> PostgreSQL version: 15.0
> Operating system:   Linux
> Description:
>
> However, the following DOES NOT work with an error of (ERROR:  generation
> expression is not immutable):
>
> * alter table test_table add created_local text GENERATED ALWAYS AS
> (EXTRACT(isoyear FROM created_at AT TIME ZONE 'America/New_York') || '|' ||
> EXTRACT(week FROM created_at AT TIME ZONE 'America/New_York')) STORED;
>
> Given that casting shouldn't "increase" the immutability of an expression,
> and expression indexes need also be immutable afaik, I think that there is
> a
> bug somewhere here?
>

Casting very much can be a non-immutable activity, dates being the prime
example, and I presume going from numeric to text is indeed defined to be
stable hence the error.  This is probably due to needing to consult locale
for deciding how to represent the decimal places divider.  This is one of
the few places, assuming you write the function to set an environment
fixing locale to some know value like you did with the time zones, where
creating an immutable function around a stable expression makes sense.

David J.

---

From: James Keener <[email-masked]>
Date: Fri, 08 Sep 2023 11:22:07 -0400

The issue here, though, is that it works as an expression for an index, but doesn't work as a generated column unless I explicitly cast it to text (which should have happened implicitly anyways). (The cast is turning a non-immutable expression to be immutable.)

I'm also able to make generated fields for the individual function calls, but concatenation doesn't work without the explicit cast.

Jim

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

---

From: Tom Lane <[email-masked]>
Date: Fri, 08 Sep 2023 12:08:19 -0400

> The issue here, though, is that it works as an expression for an index, but doesn't work as a generated column unless I explicitly cast it to text (which should have happened implicitly anyways). (…

The reason that the generated expression fails is that (if you don't
explicitly cast to text) then it relies on anytextcat(anynonarray,text),
which is only stable, and can't be marked any more restrictively because
depending on the type of the non-text argument the corresponding output
function might not be immutable.

But then why doesn't the equivalent index definition spit up?
I found the answer in indexcmds.c's CheckMutability():

    /*
     * First run the expression through the planner.  This has a couple of
     * important consequences.  First, function default arguments will get
     * inserted, which may affect volatility (consider "default now()").
     * Second, inline-able functions will get inlined, which may allow us to
     * conclude that the function is really less volatile than it's marked. As
     * an example, polymorphic functions must be marked with the most volatile
     * behavior that they have for any input type, but once we inline the
     * function we may be able to conclude that it's not so volatile for the
     * particular input type we're dealing with.
     *
     * We assume here that expression_planner() won't scribble on its input.
     */
    expr = expression_planner(expr);

    /* Now we can search for non-immutable functions */
    return contain_mutable_functions((Node *) expr);

Applying expression_planner() solves the problem because it inlines
anytextcat(anynonarray,text), resolving that the required cast is
numeric->text which is immutable.  The code for generated expressions
omits that step and arrives at the less desirable answer.  I wonder
where else we have the same issue.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Sat, 09 Sep 2023 15:18:07 -0400

[ moving to pgsql-hackers ]

> where else we have the same issue.

After digging around, I could only find one other place where
outside-the-planner code was doing this wrong: AddRelationNewConstraints
can come to the wrong conclusion about whether it's safe to use
missingMode.  So here's a patch series to resolve this.  I split it
into three parts mostly because 0002 will only go back to v12 where
we added GENERATED, but the missingMode bug exists in v11.

There are a couple of points worth bikeshedding perhaps.  I didn't
spend much thought on the wrapper functions' names, but it's surely
true that the semantic difference between contain_mutable_functions
and ContainMutableFunctions is quite un-apparent from those names.
Anybody got a better idea?  It also seemed about fifty-fifty whether
to make the wrappers' argument types be Node * or Expr *.  I stuck
with Expr * because that's what the predecessor code CheckMutability()
used, but that's not a very strong argument.

BTW, the test function in 0003 might look funny:

CREATE FUNCTION foolme(timestamptz DEFAULT clock_timestamp())
  RETURNS timestamptz
  IMMUTABLE AS 'select $1' LANGUAGE sql;

but AFAICS it's perfectly legit.  The function itself is indeed immutable,
since it's only "select $1"; it's the default argument that's volatile.

I'll add this to the open CF 2023-11, but we really ought to
get it committed before that so we can ship these bug fixes in
November's releases.

			regards, tom lane

From b8387f19c846de52083121e36c0a40cad61d594b Mon Sep 17 00:00:00 2001
From: Tom Lane <[email-masked]>
Date: Sat, 9 Sep 2023 13:54:16 -0400
Subject: [PATCH v1 1/3] Refactor to add wrappers for
 contain_mutable/volatile_functions().

contain_mutable_functions and contain_volatile_functions give
reliable answers only after expression preprocessing (specifically
eval_const_expressions).  Some places understand this, but some did
not get the memo --- which is not entirely their fault, because the
problem 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 from
copy-and-paste of code that's only conditionally safe.

This patch doesn't fix any actual bugs, just provide the infrastructure
for doing so.  The only behavioral change is that I re-ordered the
steps in ComputePartitionAttrs so that its checks for invalid column
references are done before applying expression_planner, rather than
after.  The previous coding would not complain if a partition expression
contains a disallowed column reference that gets optimized away by
constant folding, which seems to me to be a behavior we do not want.

Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/copyfrom.c      |  6 ++-
 src/backend/commands/indexcmds.c     | 31 +-------------
 src/backend/commands/tablecmds.c     | 50 +++++++++++-----------
 src/backend/optimizer/util/clauses.c | 64 ++++++++++++++++++++++++++++
 src/include/optimizer/optimizer.h    |  2 +
 5 files changed, 99 insertions(+), 54 deletions(-)

diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 70871ed819..a7ec68c25d 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -887,6 +887,9 @@ CopyFrom(CopyFromState cstate)
 		 * Can't support multi-inserts if there are any volatile function
 		 * expressions in WHERE clause.  Similarly to the trigger case above,
 		 * such expressions may query the table we're inserting into.
+		 *
+		 * Note: the whereClause was already preprocessed in DoCopy(), so it's
+		 * okay to use contain_volatile_functions() directly.
 		 */
 		insertMethod = CIM_SINGLE;
 	}
@@ -1599,7 +1602,8 @@ BeginCopyFrom(ParseState *pstate,
 				 * known to be safe for use with the multi-insert
 				 * optimization. Hence we use this special case function
 				 * checker rather than the standard check for
-				 * contain_volatile_functions().
+				 * contain_volatile_functions().  Note also that we already
+				 * ran the expression through expression_planner().
 				 */
 				if (!volatile_defexprs)
 					volatile_defexprs = contain_volatile_functions_not_nextval((Node *) defexpr);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ab8b81b302..9f9e2612cf 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1785,33 +1785,6 @@ DefineIndex(Oid tableId,
 }

-/*
- * CheckMutability
- *		Test whether given expression is mutable
- */
-static bool
-CheckMutability(Expr *expr)
-{
-	/*
-	 * First run the expression through the planner.  This has a couple of
-	 * important consequences.  First, function default arguments will get
-	 * inserted, which may affect volatility (consider "default now()").
-	 * Second, inline-able functions will get inlined, which may allow us to
-	 * conclude that the function is really less volatile than it's marked. As
-	 * an example, polymorphic functions must be marked with the most volatile
-	 * behavior that they have for any input type, but once we inline the
-	 * function we may be able to conclude that it's not so volatile for the
-	 * particular input type we're dealing with.
-	 *
-	 * We assume here that expression_planner() won't scribble on its input.
-	 */
-	expr = expression_planner(expr);
-
-	/* Now we can search for non-immutable functions */
-	return contain_mutable_functions((Node *) expr);
-}
-
-
 /*
  * CheckPredicate
  *		Checks that the given partial-index predicate is valid.
@@ -1835,7 +1808,7 @@ CheckPredicate(Expr *predicate)
 	 * A predicate using mutable functions is probably wrong, for the same
 	 * reasons that we don't allow an index expression to use one.
 	 */
-	if (CheckMutability(predicate))
+	if (ContainMutableFunctions(predicate))
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 				 errmsg("functions in index predicate must be marked IMMUTABLE")));
@@ -1978,7 +1951,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 				 * same data every time, it's not clear what the index entries
 				 * mean at all.
 				 */
-				if (CheckMutability((Expr *) expr))
+				if (ContainMutableFunctions((Expr *) expr))
 					ereport(ERROR,
 							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 							 errmsg("functions in index expression must be marked IMMUTABLE")));
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a2c671b66..dfba7abbe0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18079,30 +18079,6 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
 				partattrs[attn] = 0;	/* marks the column as expression */
 				*partexprs = lappend(*partexprs, expr);
 
-				/*
-				 * Try to simplify the expression before checking for
-				 * mutability.  The main practical value of doing it in this
-				 * order is that an inline-able SQL-language function will be
-				 * accepted if its expansion is immutable, whether or not the
-				 * function itself is marked immutable.
-				 *
-				 * Note that expression_planner does not change the passed in
-				 * expression destructively and we have already saved the
-				 * expression to be stored into the catalog above.
-				 */
-				expr = (Node *) expression_planner((Expr *) expr);
-
-				/*
-				 * Partition expression cannot contain mutable functions,
-				 * because a given row must always map to the same partition
-				 * as long as there is no change in the partition boundary
-				 * structure.
-				 */
-				if (contain_mutable_functions(expr))
-					ereport(ERROR,
-							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-							 errmsg("functions in partition key expression must be marked IMMUTABLE")));
-
 				/*
 				 * transformPartitionSpec() should have already rejected
 				 * subqueries, aggregates, window functions, and SRFs, based
@@ -18144,6 +18120,32 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
 								 parser_errposition(pstate, pelem->location)));
 				}
 
+				/*
+				 * Preprocess the expression before checking for mutability.
+				 * This is essential for the reasons described in
+				 * ContainMutableFunctions.  However, we call
+				 * expression_planner for ourselves rather than using that
+				 * function, because if constant-folding reduces the
+				 * expression to a constant, we'd like to know that so we can
+				 * complain below.
+				 *
+				 * Like ContainMutableFunctions, assume that
+				 * expression_planner won't scribble on its input, so this
+				 * doesn't affect the partexprs entry we saved above.
+				 */
+				expr = (Node *) expression_planner((Expr *) expr);
+
+				/*
+				 * Partition expressions cannot contain mutable functions,
+				 * because a given row must always map to the same partition
+				 * as long as there is no change in the partition boundary
+				 * structure.
+				 */
+				if (contain_mutable_functions(expr))
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+							 errmsg("functions in partition key expression must be marked IMMUTABLE")));
+
 				/*
 				 * While it is not exactly *wrong* for a partition expression
 				 * to be a constant, it seems better to reject such keys.
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index da258968b8..484d8e6b3f 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -360,6 +360,10 @@ contain_subplans_walker(Node *node, void *context)
  * mistakenly think that something like "WHERE random() < 0.5" can be treated
  * as a constant qualification.
  *
+ * This will give the right answer only for clauses that have been put
+ * through expression preprocessing.  Callers outside the planner typically
+ * should use ContainMutableFunctions() instead, for the reasons given there.
+ *
  * We will recursively look into Query nodes (i.e., SubLink sub-selects)
  * but not into SubPlans.  See comments for contain_volatile_functions().
  */
@@ -446,6 +450,34 @@ contain_mutable_functions_walker(Node *node, void *context)
 								  context);
 }
 
+/*
+ * ContainMutableFunctions
+ *	  Test whether given expression contains mutable functions.
+ *
+ * This is a wrapper for contain_mutable_functions() that is safe to use from
+ * outside the planner.  The difference is that it first runs the expression
+ * through expression_planner().  There are two key reasons why we need that:
+ *
+ * First, function default arguments will get inserted, which may affect
+ * volatility (consider "default now()").
+ *
+ * Second, inline-able functions will get inlined, which may allow us to
+ * conclude that the function is really less volatile than it's marked.
+ * As an example, polymorphic functions must be marked with the most volatile
+ * behavior that they have for any input type, but once we inline the
+ * function we may be able to conclude that it's not so volatile for the
+ * particular input type we're dealing with.
+ */
+bool
+ContainMutableFunctions(Expr *expr)
+{
+	/* We assume here that expression_planner() won't scribble on its input */
+	expr = expression_planner(expr);
+
+	/* Now we can search for non-immutable functions */
+	return contain_mutable_functions((Node *) expr);
+}
+
 
 /*****************************************************************************
  *		Check clauses for volatile functions
@@ -459,6 +491,10 @@ contain_mutable_functions_walker(Node *node, void *context)
  * volatile function) is found. This test prevents, for example,
  * invalid conversions of volatile expressions into indexscan quals.
  *
+ * This will give the right answer only for clauses that have been put
+ * through expression preprocessing.  Callers outside the planner typically
+ * should use ContainVolatileFunctions() instead, for the reasons given there.
+ *
  * We will recursively look into Query nodes (i.e., SubLink sub-selects)
  * but not into SubPlans.  This is a bit odd, but intentional.  If we are
  * looking at a SubLink, we are probably deciding whether a query tree
@@ -582,6 +618,34 @@ contain_volatile_functions_walker(Node *node, void *context)
 								  context);
 }
 
+/*
+ * ContainVolatileFunctions
+ *	  Test whether given expression contains volatile functions.
+ *
+ * This is a wrapper for contain_volatile_functions() that is safe to use from
+ * outside the planner.  The difference is that it first runs the expression
+ * through expression_planner().  There are two key reasons why we need that:
+ *
+ * First, function default arguments will get inserted, which may affect
+ * volatility (consider "default random()").
+ *
+ * Second, inline-able functions will get inlined, which may allow us to
+ * conclude that the function is really less volatile than it's marked.
+ * As an example, polymorphic functions must be marked with the most volatile
+ * behavior that they have for any input type, but once we inline the
+ * function we may be able to conclude that it's not so volatile for the
+ * particular input type we're dealing with.
+ */
+bool
+ContainVolatileFunctions(Expr *expr)
+{
+	/* We assume here that expression_planner() won't scribble on its input */
+	expr = expression_planner(expr);
+
+	/* Now we can search for volatile functions */
+	return contain_volatile_functions((Node *) expr);
+}
+
 /*
  * Special purpose version of contain_volatile_functions() for use in COPY:
  * ignore nextval(), but treat all other functions normally.
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 514746c585..8254da373b 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -138,7 +138,9 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
 /* in util/clauses.c: */
 
 extern bool contain_mutable_functions(Node *clause);
+extern bool ContainMutableFunctions(Expr *expr);
 extern bool contain_volatile_functions(Node *clause);
+extern bool ContainVolatileFunctions(Expr *expr);
 extern bool contain_volatile_functions_not_nextval(Node *clause);
 
 extern Node *eval_const_expressions(PlannerInfo *root, Node *node);
-- 
2.39.3

From 1e836fa15ff30c22de9b555196da3e8fbb5a2ad7 Mon Sep 17 00:00:00 2001
From: Tom Lane <[email-masked]>
Date: Sat, 9 Sep 2023 14:40:15 -0400
Subject: [PATCH v1 2/3] Preprocess column GENERATED expressions before
 checking mutability.

The previous coding skipped the preprocessing step and thus had
(at least) two failure modes: it could fail to notice the use of a
volatile function default-argument expression, or it could reject a
polymorphic function that is actually immutable on the datatype of
interest.

Per bug #18097 from Jim Keener.  Back-patch to v12 where we added
generated columns.

Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/catalog/heap.c              | 4 +++-
 src/test/regress/expected/generated.out | 3 +++
 src/test/regress/sql/generated.sql      | 3 +++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index b42711f574..9b7544e9b7 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3087,9 +3087,11 @@ cookDefault(ParseState *pstate,
 
 	if (attgenerated)
 	{
+		/* Disallow refs to other generated columns */
 		check_nested_generated(pstate, expr);
 
-		if (contain_mutable_functions(expr))
+		/* Disallow mutable functions */
+		if (ContainMutableFunctions((Expr *) expr))
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 					 errmsg("generation expression is not immutable")));
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index dc97ed3fe0..a2f38d0f50 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -61,6 +61,9 @@ LINE 1: ..._3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (c * 2) STO...
 -- generation expression must be immutable
 CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED ALWAYS AS (random()) STORED);
 ERROR:  generation expression is not immutable
+-- ... but be sure that the immutability test is accurate
+CREATE TABLE gtest2 (a int, b text GENERATED ALWAYS AS (a || ' sec') STORED);
+DROP TABLE gtest2;
 -- cannot have default/identity and generated
 CREATE TABLE gtest_err_5a (a int PRIMARY KEY, b int DEFAULT 5 GENERATED ALWAYS AS (a * 2) STORED);
 ERROR:  both default and generation expression specified for column "b" of table "gtest_err_5a"
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 8ddecf0cc3..298f6b3aa8 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -26,6 +26,9 @@ CREATE TABLE gtest_err_3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (c * 2) S
 
 -- generation expression must be immutable
 CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED ALWAYS AS (random()) STORED);
+-- ... but be sure that the immutability test is accurate
+CREATE TABLE gtest2 (a int, b text GENERATED ALWAYS AS (a || ' sec') STORED);
+DROP TABLE gtest2;
 
 -- cannot have default/identity and generated
 CREATE TABLE gtest_err_5a (a int PRIMARY KEY, b int DEFAULT 5 GENERATED ALWAYS AS (a * 2) STORED);
-- 
2.39.3

From 26cfe11c9487d53982c903d929d9686fecf04911 Mon Sep 17 00:00:00 2001
From: Tom Lane <[email-masked]>
Date: Sat, 9 Sep 2023 14:53:24 -0400
Subject: [PATCH v1 3/3] Preprocess column DEFAULT expressions before checking
 volatility.

The previous coding skipped the preprocessing step and thus had
(at least) two failure modes: it could fail to notice the use of a
volatile function default-argument expression, or it could reject a
polymorphic function that is actually immutable on the datatype of
interest.  The latter would just result in an unnecessary table
rewrite, but the former could allow the attmissingval functionality
to be used in a case where it should not be.

Noted while investigating bug #18097 from Jim Keener.  Back-patch
to all supported versions.

Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/catalog/heap.c                 |  2 +-
 src/test/regress/expected/fast_default.out | 18 ++++++++++++++++++
 src/test/regress/sql/fast_default.sql      | 11 +++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 9b7544e9b7..b44a4c96bb 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2365,7 +2365,7 @@ AddRelationNewConstraints(Relation rel,
 			continue;
 
 		/* If the DEFAULT is volatile we cannot use a missing value */
-		if (colDef->missingMode && contain_volatile_functions((Node *) expr))
+		if (colDef->missingMode && ContainVolatileFunctions((Expr *) expr))
 			colDef->missingMode = false;
 
 		defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index 91f25717b5..59365dad96 100644
--- a/src/test/regress/expected/fast_default.out
+++ b/src/test/regress/expected/fast_default.out
@@ -272,7 +272,25 @@ SELECT comp();
  Rewritten
 (1 row)
 
+-- check that we notice insertion of a volatile default argument
+CREATE FUNCTION foolme(timestamptz DEFAULT clock_timestamp())
+  RETURNS timestamptz
+  IMMUTABLE AS 'select $1' LANGUAGE sql;
+ALTER TABLE T ADD COLUMN c3 timestamptz DEFAULT foolme();
+NOTICE:  rewriting table t for reason 2
+SELECT attname, atthasmissing, attmissingval FROM pg_attribute
+  WHERE attrelid = 't'::regclass AND attnum > 0
+  ORDER BY attnum;
+ attname | atthasmissing | attmissingval 
+---------+---------------+---------------
+ pk      | f             | 
+ c1      | f             | 
+ c2      | f             | 
+ c3      | f             | 
+(4 rows)
+
 DROP TABLE T;
+DROP FUNCTION foolme(timestamptz);
 -- Simple querie
 CREATE TABLE T (pk INT NOT NULL PRIMARY KEY);
 SELECT set('t');
diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql
index 16a3b7ca51..dc9df78a35 100644
--- a/src/test/regress/sql/fast_default.sql
+++ b/src/test/regress/sql/fast_default.sql
@@ -256,7 +256,18 @@ ALTER TABLE T ADD COLUMN c2 TIMESTAMP DEFAULT clock_timestamp();
 
 SELECT comp();
 
+-- check that we notice insertion of a volatile default argument
+CREATE FUNCTION foolme(timestamptz DEFAULT clock_timestamp())
+  RETURNS timestamptz
+  IMMUTABLE AS 'select $1' LANGUAGE sql;
+ALTER TABLE T ADD COLUMN c3 timestamptz DEFAULT foolme();
+
+SELECT attname, atthasmissing, attmissingval FROM pg_attribute
+  WHERE attrelid = 't'::regclass AND attnum > 0
+  ORDER BY attnum;
+
 DROP TABLE T;
+DROP FUNCTION foolme(timestamptz);
 
 -- Simple querie
 CREATE TABLE T (pk INT NOT NULL PRIMARY KEY);
-- 
2.39.3

---

From: Tom Lane <[email-masked]>
Date: Sat, 09 Sep 2023 18:56:42 -0400

I wrote:
> missingMode.  So here's a patch series to resolve this.

Argh ... I forgot to mention that there's one other place that
this patch series doesn't address, which is that publicationcmds.c's
check_simple_rowfilter_expr() also checks for volatile functions
without having preprocessed the expression.  I'm not entirely sure
that there's a reachable problem in the direction of underestimating
the expression's volatility, given that that logic rejects non-builtin
functions entirely: it seems unlikely that any immutable builtin
function would have a volatile default expression.  But it definitely
seems possible that there would be a problem in the other direction,
leading to rejecting row filter expressions that we'd like to allow,
much as in bug #18097.

I'm not sure about a good way to resolve this.  Simply applying
expression simplification ahead of the check would break the code's
intent of rejecting non-builtin operators, in the case where such
an operator resolves to an inline-able builtin function.  I find
the entire design of check_simple_rowfilter_expr pretty questionable
anyway, and there are a bunch of dubious (and undocumented) individual
decisions like allowing whole-row Vars, using FirstNormalObjectId
rather than FirstUnpinnedObjectId as the cutoff, etc.  So I'm not
planning to touch that code, but somebody who was paying attention
when it was written might want to take a second look.

			regards, tom lane

---

From: Aleksander Alekseev <[email-masked]>
Date: Tue, 14 Nov 2023 15:10:05 +0300

Hi,

I noticed that the patchset needs a review and decided to take a look.

> Anybody got a better idea?

Oh no! We encountered one of the most difficult problems in computer
science [1].

ContainMutableFunctionsAfterPerformingPlannersTransformations() would
be somewhat long but semantically correct. It can be shortened to
ContainMutableFunctionsAfterTransformations() or perhaps
TransformedExprContainMutableFunctions(). Personally I don't mind long
names. This being said, ContainMutableFunctions() doesn't disgusts my
sense of beauty too much either. All in all any name will do IMO.
Naturally ContainVolatileFunctions() should be renamed consistently
with ContainMutableFunctions().

I couldn't find anything wrong with 0001..0003. The parches were
tested in several environments and passed `make check-world`. I
suggest merging them.

[1]: https://martinfowler.com/bliki/TwoHardThings.html

-- 
Best regards,
Aleksander Alekseev

---

From: Tom Lane <[email-masked]>
Date: Tue, 14 Nov 2023 12:48:54 -0500

> Oh no! We encountered one of the most difficult problems in computer

Indeed :-(.  Looking at it again this morning, I'm thinking of
using "contain_mutable_functions_after_planning" --- what do you
think of that?

			regards, tom lane