From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Cc: i.portnov@compassplus.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19487: Error while executing SQL query involving XML parsing
Date: Sun, 30 Aug 2026 14:03:58 -0400
Message-ID: <880213.1788113038@sss.pgh.pa.us> (raw)
In-Reply-To: <CAB8bMivhR6_d33YFc-cAKT-Qa8Vj8MHicVUexyJSySTi+c4k_g@mail.gmail.com>
References: <19487-367258bc497b923a@postgresql.org>
<CAB8bMisngV0L9L3dZkpuStTboHXkVPr0=NBhCNpDxg3r+Bheog@mail.gmail.com>
<1589702.1787946553@sss.pgh.pa.us>
<CAB8bMitYCBLs16zVzNLgB34go2s4p9mraGsvcuDeUKyO+a4=cA@mail.gmail.com>
<CAB8bMisnPP=Ck=BX4UfHL1eTFLMFJ_RznsVXtqy3H=P+6tugjg@mail.gmail.com>
<255521.1788039871@sss.pgh.pa.us>
<CAB8bMivhR6_d33YFc-cAKT-Qa8Vj8MHicVUexyJSySTi+c4k_g@mail.gmail.com>
Andrey Rachitskiy <pl0h0yp1@gmail.com> writes:
> I think you're right. Fixed it in v3.
I spent some time looking at this, and felt that what we really
ought to do is update clauses.c's handling of XmlExpr across the
board. In particular, contain_mutable_functions really shouldn't
allow XmlExpr in cases where we know it's not immutable. So more
or less as attached, where I also spent more effort on the comments.
I've got mixed emotions about back-patching this. It's not really
a bug fix, because we already disclaim the safety of relying on
CASE for this purpose, cf [1]. I think it might still be reasonable
to sneak into v19, but I'm unsure that putting it into 18 is a good
idea.
regards, tom lane
[1] https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-EXPRESS-EVALAttachments:
[text/x-diff] v4-0001-Handle-XmlExpr-more-honestly-in-clauses.c.patch (7.7K, ../880213.1788113038@sss.pgh.pa.us/2-v4-0001-Handle-XmlExpr-more-honestly-in-clauses.c.patch)
download | inline diff:
From 1cf565ab1665c57f12047ce294700d5c17cecb7b Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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 <i.portnov@compassplus.com>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
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.cindex 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.outindex 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.sqlindex aafd39433a6..ea0438aa45d 100644--- a/src/test/regress/sql/xml.sql+++ b/src/test/regress/sql/xml.sql@@ -185,6 +185,13 @@ SELECT xml '<abc/>' 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, pl0h0yp1@gmail.com, i.portnov@compassplus.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19487: Error while executing SQL query involving XML parsing
In-Reply-To: <880213.1788113038@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox