Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1x0jtC-004mtN-0C for pgsql-bugs@arkaria.postgresql.org; Sun, 30 Aug 2026 18:04:18 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1x0jtB-00ECWs-03 for pgsql-bugs@arkaria.postgresql.org; Sun, 30 Aug 2026 18:04:17 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1x0jtA-00ECWk-22 for pgsql-bugs@lists.postgresql.org; Sun, 30 Aug 2026 18:04:16 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1x0jt7-00000003BJA-2a86 for pgsql-bugs@lists.postgresql.org; Sun, 30 Aug 2026 18:04:15 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.18.1/8.18.1) with ESMTP id 67UI3wTg880214; Sun, 30 Aug 2026 14:03:58 -0400 From: Tom Lane To: Andrey Rachitskiy cc: i.portnov@compassplus.com, pgsql-bugs@lists.postgresql.org Subject: Re: BUG #19487: Error while executing SQL query involving XML parsing In-reply-to: References: <19487-367258bc497b923a@postgresql.org> <1589702.1787946553@sss.pgh.pa.us> <255521.1788039871@sss.pgh.pa.us> Comments: In-reply-to Andrey Rachitskiy message dated "Sun, 30 Aug 2026 08:08:19 +0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <880197.1788113026.0@sss.pgh.pa.us> Date: Sun, 30 Aug 2026 14:03:58 -0400 Message-ID: <880213.1788113038@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <880197.1788113026.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Andrey Rachitskiy 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-EX= PRESS-EVAL ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v4-0001-Handle-XmlExpr-more-honestly-in-clauses.c.patch"; charset="us-ascii" Content-ID: <880197.1788113026.2@sss.pgh.pa.us> Content-Description: v4-0001-Handle-XmlExpr-more-honestly-in-clauses.c.patch Content-Transfer-Encoding: quoted-printable =46rom 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 *lis= ts); 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, voi= d *context); static bool max_parallel_hazard_walker(Node *node, @@ -468,6 +469,13 @@ contain_mutable_functions_walker(Node *node, void *co= ntext) 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 *c= ontext) /* * It should be safe to treat MinMaxExpr as immutable, because it will * depend on a non-cross-type btree comparison function, and those shoul= d - * 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 mor= e - * 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 outrigh= t + * 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 *co= ntext) 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 *co= ntext) = /* * 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 =3D 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 ------- =_aaaaaaaaaa0--