From 1cf565ab1665c57f12047ce294700d5c17cecb7b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 30 Aug 2026 13:55:06 -0400 Subject: [PATCH v4] Handle XmlExpr more honestly in clauses.c. eval_const_expressions() didn't constant-fold XmlExpr. In some cases it cannot, but often it can. Failure to do so can cause visible regressions in SQL-language function behavior compared to pre-v18, though only in a rather narrow set of contexts: basically, if you're trying to use a CASE to prevent evaluation of a dangerous function. While we're at it, make contain_mutable_functions() handle XmlExpr more precisely, and adjust some comments that justify not treating XmlExpr explicitly. Bug: #19487 Reported-by: Ilya Portnov Author: Andrey Rachitskiy Co-authored-by: Tom Lane Discussion: https://postgr.es/m/19487-367258bc497b923a@postgresql.org Backpatch-through: ?? --- src/backend/optimizer/util/clauses.c | 83 ++++++++++++++++++++++++---- src/test/regress/expected/xml.out | 11 ++++ src/test/regress/sql/xml.sql | 7 +++ 3 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 8da4ed617b5..737bd4a4fdf 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -115,6 +115,7 @@ static bool contain_agg_clause_walker(Node *node, void *context); static bool find_window_functions_walker(Node *node, WindowFuncLists *lists); static bool contain_subplans_walker(Node *node, void *context); static bool contain_mutable_functions_walker(Node *node, void *context); +static bool xmlexpr_is_immutable(XmlExpr *xexpr); static bool contain_volatile_functions_walker(Node *node, void *context); static bool contain_volatile_functions_not_nextval_walker(Node *node, void *context); static bool max_parallel_hazard_walker(Node *node, @@ -468,6 +469,13 @@ contain_mutable_functions_walker(Node *node, void *context) return true; } + if (IsA(node, XmlExpr)) + { + /* some variants of XmlExpr are only stable */ + if (!xmlexpr_is_immutable((XmlExpr *) node)) + return true; + } + if (IsA(node, NextValueExpr)) { /* NextValueExpr is volatile */ @@ -477,11 +485,10 @@ contain_mutable_functions_walker(Node *node, void *context) /* * It should be safe to treat MinMaxExpr as immutable, because it will * depend on a non-cross-type btree comparison function, and those should - * always be immutable. Treating XmlExpr as immutable is more dubious, - * and treating CoerceToDomain as immutable is outright dangerous. But we - * have done so historically, and changing this would probably cause more - * problems than it would fix. In practice, if you have a non-immutable - * domain constraint you are in for pain anyhow. + * always be immutable. Treating CoerceToDomain as immutable is outright + * dangerous, but we have done so historically, and changing this would + * probably cause more problems than it would fix. In practice, if you + * have a non-immutable domain constraint you are in for pain anyhow. */ /* Recurse to check arguments */ @@ -496,6 +503,44 @@ contain_mutable_functions_walker(Node *node, void *context) context); } +/* + * xmlexpr_is_immutable + * True if this XmlExpr node represents immutable processing + * (considering just the node itself, not its arguments) + */ +static bool +xmlexpr_is_immutable(XmlExpr *xexpr) +{ + switch (xexpr->op) + { + case IS_XMLCONCAT: + case IS_XMLPARSE: + case IS_XMLPI: + case IS_XMLROOT: + case IS_XMLSERIALIZE: + case IS_DOCUMENT: + /* These variants manipulate XML text in a self-contained way */ + return true; + + case IS_XMLELEMENT: + case IS_XMLFOREST: + + /* + * These variants invoke I/O conversion functions for a wide range + * of data types, and have various special rules too, so in some + * cases they are only stable. In principle we could analyze + * their behavior precisely, but keeping such code in sync with + * the actual implementation seems like more maintenance risk than + * it's worth. + */ + return false; + + /* There is intentionally no default: case here */ + } + /* We shouldn't get here, but if we do, say "not immutable" */ + return false; +} + /* * contain_mutable_functions_after_planning * Test whether given expression contains mutable functions. @@ -649,8 +694,9 @@ contain_volatile_functions_walker(Node *node, void *context) /* * See notes in contain_mutable_functions_walker about why we treat - * MinMaxExpr, XmlExpr, and CoerceToDomain as immutable, while - * SQLValueFunction is stable. Hence, none of them are of interest here. + * MinMaxExpr and CoerceToDomain as immutable. SQLValueFunction is + * stable, and XmlExpr might be immutable or stable, but it should never + * be volatile. Hence, none of them are of interest here. */ /* Recurse to check arguments */ @@ -723,10 +769,11 @@ contain_volatile_functions_not_nextval_walker(Node *node, void *context) /* * See notes in contain_mutable_functions_walker about why we treat - * MinMaxExpr, XmlExpr, and CoerceToDomain as immutable, while - * SQLValueFunction is stable. Hence, none of them are of interest here. - * Also, since we're intentionally ignoring nextval(), presumably we - * should ignore NextValueExpr. + * MinMaxExpr and CoerceToDomain as immutable. SQLValueFunction is + * stable, and XmlExpr might be immutable or stable, but it should never + * be volatile. Hence, none of them are of interest here. Also, since + * we're intentionally ignoring nextval(), presumably we should ignore + * NextValueExpr. */ /* Recurse to check arguments */ @@ -3786,6 +3833,20 @@ eval_const_expressions_mutator(Node *node, else return copyObject((Node *) svf); } + case T_XmlExpr: + { + /* + * Some variants of XmlExpr are immutable. Others are only + * stable, but in estimation mode those are still fair game to + * simplify. + */ + node = ece_generic_processing(node); + if ((context->estimate || + xmlexpr_is_immutable((XmlExpr *) node)) && + ece_all_arguments_const(node)) + return ece_evaluate_expr(node); + return node; + } case T_FieldSelect: { /* diff --git a/src/test/regress/expected/xml.out b/src/test/regress/expected/xml.out index fb3e0ec41b2..350941f7172 100644 --- a/src/test/regress/expected/xml.out +++ b/src/test/regress/expected/xml.out @@ -713,6 +713,17 @@ LINE 1: SELECT '<>' IS NOT DOCUMENT; DETAIL: line 1: StartTag: invalid element name <> ^ +-- Check that IS DOCUMENT is known immutable, so that we don't reach the +-- incorrect xpath() call at plan time. +SELECT CASE WHEN ('2019-12-16T00:00:00.000'::xml) IS DOCUMENT + THEN (xpath('/*/text()', '2019-12-16T00:00:00.000'::xml))[1] + ELSE '2019-12-16T00:00:00.000'::xml +END; + case +------------------------- + 2019-12-16T00:00:00.000 +(1 row) + SELECT xmlagg(data) FROM xmltest; xmlagg -------------------------------------- diff --git a/src/test/regress/sql/xml.sql b/src/test/regress/sql/xml.sql index aafd39433a6..ea0438aa45d 100644 --- a/src/test/regress/sql/xml.sql +++ b/src/test/regress/sql/xml.sql @@ -185,6 +185,13 @@ SELECT xml '' IS NOT DOCUMENT; SELECT xml 'abc' IS NOT DOCUMENT; SELECT '<>' IS NOT DOCUMENT; +-- Check that IS DOCUMENT is known immutable, so that we don't reach the +-- incorrect xpath() call at plan time. +SELECT CASE WHEN ('2019-12-16T00:00:00.000'::xml) IS DOCUMENT + THEN (xpath('/*/text()', '2019-12-16T00:00:00.000'::xml))[1] + ELSE '2019-12-16T00:00:00.000'::xml +END; + SELECT xmlagg(data) FROM xmltest; SELECT xmlagg(data) FROM xmltest WHERE id > 10; -- 2.52.0