database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2026-10-23 13:29:09 +03:00
Code Activity

Handle XmlExpr more honestly in clauses.c.

Browse Source 
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 <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: 19
This commit is contained in:
Tom Lane
2026-09-01 12:16:45 -04:00
parent a4da804193
commit f3ac68ffe3
3 changed files with 90 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
src/backend/optimizer/util/clauses.c
View File

@@ -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:
{
/*

11
src/test/regress/expected/xml.out
View File

@@ -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
--------------------------------------

7
src/test/regress/sql/xml.sql
View File

@@ -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;