From f3ac68ffe38692582c24880ef10cc365ec683fa6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 1 Sep 2026 12:16:45 -0400 Subject: [PATCH] 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 failure-prone XML function. While we're at it, make contain_mutable_functions() handle XmlExpr more precisely, and adjust some comments that justify not treating XmlExpr explicitly. This seems worth sneaking into v19, but I'm hesitant to put it into v18. It's not really a bug fix, because we disclaim the safety of using CASE this way, and it carries some risk of de-optimizing queries that worked satisfactorily before. 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: 19 --- 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 337fc27262e..329c22bf133 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, @@ -469,6 +470,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 */ @@ -478,11 +486,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 */ @@ -497,6 +504,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. @@ -650,8 +695,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 */ @@ -724,10 +770,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 */ @@ -3745,6 +3792,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;