Missing [NO] INDENT flag in XMLSerialize backward parsing
Hi,
This patch adds the missing [NO] INDENT flag to XMLSerialize backward
parsing. For example:
CREATE VIEW v1 AS
SELECT
xmlserialize(
DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text
INDENT);
\sv v1
CREATE OR REPLACE VIEW public.v1 AS
SELECT XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS
text INDENT) AS "xmlserialize"
SELECT * FROM v1;
xmlserialize
-----------------
<foo> +
<bar>42</bar>+
</foo>
(1 row)
The NO INDENT flag is added by default if no explicit indentation
flag was originally provided:
CREATE VIEW v2 AS
SELECT
xmlserialize(
DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text
NO INDENT);
\sv v2
CREATE OR REPLACE VIEW public.v2 AS
SELECT XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text NO
INDENT) AS "xmlserialize"
CREATE VIEW v3 AS
SELECT
xmlserialize(
DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text);
\sv v3
CREATE OR REPLACE VIEW public.v3 AS
SELECT XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text NO
INDENT) AS "xmlserialize"
Regression tests were updated accordingly.
Best regards, Jim
Attachments:
v1-0001-Fix-missing-NO-INDENT-flag-in-XMLSerialize-backwa.patchtext/x-patch; charset=UTF-8; name=v1-0001-Fix-missing-NO-INDENT-flag-in-XMLSerialize-backwa.patchDownload
From ff1b261ef74ae381eb946ec675e0f116c728cee0 Mon Sep 17 00:00:00 2001
From: Jim Jones <jim.jones@uni-muenster.de>
Date: Thu, 20 Feb 2025 14:08:04 +0100
Subject: [PATCH v1] Fix missing [NO] INDENT flag in XMLSerialize backward
parsing
This patch adds the missing [NO] INDENT flag to XMLSerialize
backward parsing. For example:
CREATE VIEW v1 AS
SELECT
xmlserialize(
DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text
INDENT);
\sv v1
CREATE OR REPLACE VIEW public.v1 AS
SELECT XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS
text INDENT) AS "xmlserialize"
SELECT * FROM v1;
xmlserialize
-----------------
<foo> +
<bar>42</bar>+
</foo>
(1 row)
The NO INDENT flag is added by default if no explicit indentation
flag was originally provided. Regression tests have been updated.
Regression tests were updated accordingly.
---
src/backend/utils/adt/ruleutils.c | 7 +++++++
src/test/regress/expected/xml.out | 16 ++++++++++------
src/test/regress/expected/xml_1.out | 10 ++++++++++
src/test/regress/expected/xml_2.out | 16 ++++++++++------
src/test/regress/sql/xml.sql | 2 ++
5 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 54dad97555..d11a8a20ee 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -10142,9 +10142,16 @@ get_rule_expr(Node *node, deparse_context *context,
}
}
if (xexpr->op == IS_XMLSERIALIZE)
+ {
appendStringInfo(buf, " AS %s",
format_type_with_typemod(xexpr->type,
xexpr->typmod));
+ if (xexpr->indent)
+ appendStringInfoString(buf, " INDENT");
+ else
+ appendStringInfoString(buf, " NO INDENT");
+ }
+
if (xexpr->op == IS_DOCUMENT)
appendStringInfoString(buf, " IS DOCUMENT");
else
diff --git a/src/test/regress/expected/xml.out b/src/test/regress/expected/xml.out
index 2e9616acda..bcc743f485 100644
--- a/src/test/regress/expected/xml.out
+++ b/src/test/regress/expected/xml.out
@@ -822,21 +822,25 @@ CREATE VIEW xmlview6 AS SELECT xmlpi(name foo, 'bar');
CREATE VIEW xmlview7 AS SELECT xmlroot(xml '<foo/>', version no value, standalone yes);
CREATE VIEW xmlview8 AS SELECT xmlserialize(content 'good' as char(10));
CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
+CREATE VIEW xmlview10 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS text indent);
+CREATE VIEW xmlview11 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS character varying no indent);
SELECT table_name, view_definition FROM information_schema.views
WHERE table_name LIKE 'xmlview%' ORDER BY 1;
- table_name | view_definition
-------------+------------------------------------------------------------------------------------------------------------
+ table_name | view_definition
+------------+---------------------------------------------------------------------------------------------------------------------------------------
xmlview1 | SELECT xmlcomment('test'::text) AS xmlcomment;
+ xmlview10 | SELECT XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text INDENT) AS "xmlserialize";
+ xmlview11 | SELECT (XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS character varying NO INDENT))::character varying AS "xmlserialize";
xmlview2 | SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
xmlview3 | SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS "xmlelement";
- xmlview4 | SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement" +
+ xmlview4 | SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement" +
| FROM emp;
xmlview5 | SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
xmlview6 | SELECT XMLPI(NAME foo, 'bar'::text) AS "xmlpi";
xmlview7 | SELECT XMLROOT('<foo/>'::xml, VERSION NO VALUE, STANDALONE YES) AS "xmlroot";
- xmlview8 | SELECT (XMLSERIALIZE(CONTENT 'good'::xml AS character(10)))::character(10) AS "xmlserialize";
- xmlview9 | SELECT XMLSERIALIZE(CONTENT 'good'::xml AS text) AS "xmlserialize";
-(9 rows)
+ xmlview8 | SELECT (XMLSERIALIZE(CONTENT 'good'::xml AS character(10) NO INDENT))::character(10) AS "xmlserialize";
+ xmlview9 | SELECT XMLSERIALIZE(CONTENT 'good'::xml AS text NO INDENT) AS "xmlserialize";
+(11 rows)
-- Text XPath expressions evaluation
SELECT xpath('/value', data) FROM xmltest;
diff --git a/src/test/regress/expected/xml_1.out b/src/test/regress/expected/xml_1.out
index 7505a14077..a1c5d31417 100644
--- a/src/test/regress/expected/xml_1.out
+++ b/src/test/regress/expected/xml_1.out
@@ -583,6 +583,16 @@ ERROR: unsupported XML feature
LINE 1: ...EATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as ...
^
DETAIL: This functionality requires the server to be built with libxml support.
+CREATE VIEW xmlview10 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS text indent);
+ERROR: unsupported XML feature
+LINE 1: ...TE VIEW xmlview10 AS SELECT xmlserialize(document '<foo><bar...
+ ^
+DETAIL: This functionality requires the server to be built with libxml support.
+CREATE VIEW xmlview11 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS character varying no indent);
+ERROR: unsupported XML feature
+LINE 1: ...TE VIEW xmlview11 AS SELECT xmlserialize(document '<foo><bar...
+ ^
+DETAIL: This functionality requires the server to be built with libxml support.
SELECT table_name, view_definition FROM information_schema.views
WHERE table_name LIKE 'xmlview%' ORDER BY 1;
table_name | view_definition
diff --git a/src/test/regress/expected/xml_2.out b/src/test/regress/expected/xml_2.out
index c07ed2b269..045641dae6 100644
--- a/src/test/regress/expected/xml_2.out
+++ b/src/test/regress/expected/xml_2.out
@@ -808,21 +808,25 @@ CREATE VIEW xmlview6 AS SELECT xmlpi(name foo, 'bar');
CREATE VIEW xmlview7 AS SELECT xmlroot(xml '<foo/>', version no value, standalone yes);
CREATE VIEW xmlview8 AS SELECT xmlserialize(content 'good' as char(10));
CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
+CREATE VIEW xmlview10 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS text indent);
+CREATE VIEW xmlview11 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS character varying no indent);
SELECT table_name, view_definition FROM information_schema.views
WHERE table_name LIKE 'xmlview%' ORDER BY 1;
- table_name | view_definition
-------------+------------------------------------------------------------------------------------------------------------
+ table_name | view_definition
+------------+---------------------------------------------------------------------------------------------------------------------------------------
xmlview1 | SELECT xmlcomment('test'::text) AS xmlcomment;
+ xmlview10 | SELECT XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS text INDENT) AS "xmlserialize";
+ xmlview11 | SELECT (XMLSERIALIZE(DOCUMENT '<foo><bar>42</bar></foo>'::xml AS character varying NO INDENT))::character varying AS "xmlserialize";
xmlview2 | SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
xmlview3 | SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS "xmlelement";
- xmlview4 | SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement" +
+ xmlview4 | SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement" +
| FROM emp;
xmlview5 | SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
xmlview6 | SELECT XMLPI(NAME foo, 'bar'::text) AS "xmlpi";
xmlview7 | SELECT XMLROOT('<foo/>'::xml, VERSION NO VALUE, STANDALONE YES) AS "xmlroot";
- xmlview8 | SELECT (XMLSERIALIZE(CONTENT 'good'::xml AS character(10)))::character(10) AS "xmlserialize";
- xmlview9 | SELECT XMLSERIALIZE(CONTENT 'good'::xml AS text) AS "xmlserialize";
-(9 rows)
+ xmlview8 | SELECT (XMLSERIALIZE(CONTENT 'good'::xml AS character(10) NO INDENT))::character(10) AS "xmlserialize";
+ xmlview9 | SELECT XMLSERIALIZE(CONTENT 'good'::xml AS text NO INDENT) AS "xmlserialize";
+(11 rows)
-- Text XPath expressions evaluation
SELECT xpath('/value', data) FROM xmltest;
diff --git a/src/test/regress/sql/xml.sql b/src/test/regress/sql/xml.sql
index bac0388ac1..4c3520ce89 100644
--- a/src/test/regress/sql/xml.sql
+++ b/src/test/regress/sql/xml.sql
@@ -219,6 +219,8 @@ CREATE VIEW xmlview6 AS SELECT xmlpi(name foo, 'bar');
CREATE VIEW xmlview7 AS SELECT xmlroot(xml '<foo/>', version no value, standalone yes);
CREATE VIEW xmlview8 AS SELECT xmlserialize(content 'good' as char(10));
CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
+CREATE VIEW xmlview10 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS text indent);
+CREATE VIEW xmlview11 AS SELECT xmlserialize(document '<foo><bar>42</bar></foo>' AS character varying no indent);
SELECT table_name, view_definition FROM information_schema.views
WHERE table_name LIKE 'xmlview%' ORDER BY 1;
--
2.34.1
On Thu, Feb 20, 2025 at 02:27:42PM +0100, Jim Jones wrote:
This patch adds the missing [NO] INDENT flag to XMLSerialize backward
parsing.
if (xexpr->op == IS_XMLSERIALIZE)
+ {
appendStringInfo(buf, " AS %s",
format_type_with_typemod(xexpr->type,
xexpr->typmod));
+ if (xexpr->indent)
+ appendStringInfoString(buf, " INDENT");
+ else
+ appendStringInfoString(buf, " NO INDENT");
+ }Good catch, we are forgetting this option in ruleutils.c. Will fix
down to v16 where this option has been introduced as you are
proposing, with NO INDENT showing up in the default case. The three
expected outputs look OK as written..
--
Michael
On 2025-02-21 Fr 1:31 AM, Michael Paquier wrote:
On Thu, Feb 20, 2025 at 02:27:42PM +0100, Jim Jones wrote:
This patch adds the missing [NO] INDENT flag to XMLSerialize backward
parsing.if (xexpr->op == IS_XMLSERIALIZE) + { appendStringInfo(buf, " AS %s", format_type_with_typemod(xexpr->type, xexpr->typmod)); + if (xexpr->indent) + appendStringInfoString(buf, " INDENT"); + else + appendStringInfoString(buf, " NO INDENT"); + }Good catch, we are forgetting this option in ruleutils.c. Will fix
down to v16 where this option has been introduced as you are
proposing, with NO INDENT showing up in the default case. The three
expected outputs look OK as written..
The fix has broken cross version upgrade test. Maybe we need to filter
out NO INDENT in releases prior to 16 in AdjustUpgrade.pm?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Feb 21, 2025 at 04:36:07AM -0500, Andrew Dunstan wrote:
The fix has broken cross version upgrade test. Maybe we need to filter out
NO INDENT in releases prior to 16 in AdjustUpgrade.pm?
Yes, I was just looking at that. The regex I am finishing with in
AdjustUpgrade.pm is something like that, which is enough to discard
the NO INDENT clause in an XMLSERIALIZE:
--- src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
+++ src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
@@ -628,6 +628,12 @@ sub adjust_new_dumpfile
\s+FUNCTION\s2\s\(text,\stext\)\spublic\.part_hashtext_length\(text,bigint\);} {}mxg;
}+ # pre-v16 dumps do not know about XMLSERIALIZE(NO INDENT).
+ if ($old_version < 16)
+ {
+ $dump =~ s/XMLSERIALIZE\((.*)? NO INDENT\)/XMLSERIALIZE\($1\)/mg;
+ }This needs to be applied in adjust_new_dumpfile() so as the comparison
with the old dump will be stable, is that right?
--
Michael
On Feb 21, 2025, at 4:55 AM, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Feb 21, 2025 at 04:36:07AM -0500, Andrew Dunstan wrote:
The fix has broken cross version upgrade test. Maybe we need to filter out
NO INDENT in releases prior to 16 in AdjustUpgrade.pm?sYes, I was just looking at that. The regex I am finishing with in AdjustUpgrade.pm is something like that, which is enough to discard the NO INDENT clause in an XMLSERIALIZE: --- src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm +++ src/test/perl/PostgreSQL/Test/AdjustUpgrade @@ -628,6 +628,12 @@ sub adjust_new_dumpfile \s+FUNCTION\s2\s\(text,\stext\)\spublic\.part_hashtext_length\(text,bigint\);} {}mxg; }+ # pre-v16 dumps do not know about XMLSERIALIZE(NO INDENT). + if ($old_version < 16) + { + $dump =~ s/XMLSERIALIZE\((.*)? NO INDENT\)/XMLSERIALIZE\($1\)/mg; + }This needs to be applied in adjust_new_dumpfile() so as the comparison
with the old dump will be stable, is that right?
I think so. Looks good to me
Cheers
Andrew
Hi Michael & Andrew
On 21.02.25 11:46, Andrew Dunstan wrote:
On Feb 21, 2025, at 4:55 AM, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Feb 21, 2025 at 04:36:07AM -0500, Andrew Dunstan wrote:
The fix has broken cross version upgrade test. Maybe we need to filter out
NO INDENT in releases prior to 16 in AdjustUpgrade.pm?sYes, I was just looking at that. The regex I am finishing with in AdjustUpgrade.pm is something like that, which is enough to discard the NO INDENT clause in an XMLSERIALIZE: --- src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm +++ src/test/perl/PostgreSQL/Test/AdjustUpgrade @@ -628,6 +628,12 @@ sub adjust_new_dumpfile \s+FUNCTION\s2\s\(text,\stext\)\spublic\.part_hashtext_length\(text,bigint\);} {}mxg; }+ # pre-v16 dumps do not know about XMLSERIALIZE(NO INDENT). + if ($old_version < 16) + { + $dump =~ s/XMLSERIALIZE\((.*)? NO INDENT\)/XMLSERIALIZE\($1\)/mg; + }This needs to be applied in adjust_new_dumpfile() so as the comparison
with the old dump will be stable, is that right?I think so. Looks good to me
Thanks for the quick response!
For future reference, what’s the best way to verify this myself? The
buildfarm was all green.
Best regards, Jim
On Fri, Feb 21, 2025 at 05:46:52AM -0500, Andrew Dunstan wrote:
I think so. Looks good to me
Thanks. I have used that after a second check. Let's see how it
goes.
--
Michael
On Fri, Feb 21, 2025 at 12:29:12PM +0100, Jim Jones wrote:
For future reference, what’s the best way to verify this myself? The
buildfarm was all green.
Don't worry, we are all trapped by that at some point.
One way would be to generate by yourself dumps from an older version
by yourself, as documented by src/bin/pg_upgrade/TESTING, part DETAILS
(the part about USE_MODULE_DB=1 is very important). Then you can
reuse the SQL files by specifying an old dump files in the TAP tests
of pg_upgrade. The buildfarm does exactly that.
For every commit, doing this much testing is a bit annoying, and I
face this issue less than once a year for anything I apply. So let's
say that I trust the buildfarm to report back at this stage ;)
--
Michael
On 21.02.25 12:46, Michael Paquier wrote:
One way would be to generate by yourself dumps from an older version
by yourself, as documented by src/bin/pg_upgrade/TESTING, part DETAILS
(the part about USE_MODULE_DB=1 is very important). Then you can
reuse the SQL files by specifying an old dump files in the TAP tests
of pg_upgrade. The buildfarm does exactly that.
Alright, I’ll keep this in mind next time I write something involving
multiple major versions.
Thanks!
Jim
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Feb 21, 2025 at 12:29:12PM +0100, Jim Jones wrote:
For future reference, what’s the best way to verify this myself? The
buildfarm was all green.
Don't worry, we are all trapped by that at some point.
Yeah, I leave that to the buildfarm too.
If I have to actually debug or test behavior touching this, I use a
non-reporting buildfarm instance that I have lying around, which is
configured to run the cross-version tests and not very much else.
I can switch that between pulling source from the community repo
and pulling from a local checkout that has the mods I want to test.
regards, tom lane
On Fri, Feb 21, 2025 at 08:38:55PM +0900, Michael Paquier wrote:
Thanks. I have used that after a second check. Let's see how it
goes.
The buildfarm has turned green on v16 and v17. It is still red
because of a different issue, while mine is gone. So we should be
good.
--
Michael
On Fri, Feb 21, 2025 at 09:53:06AM -0500, Tom Lane wrote:
If I have to actually debug or test behavior touching this, I use a
non-reporting buildfarm instance that I have lying around, which is
configured to run the cross-version tests and not very much else.
I can switch that between pulling source from the community repo
and pulling from a local checkout that has the mods I want to test.
Another simpler method that I have used here: grab the diff report
from the buildfarm and use a small perl script to build and checj the
regex that would be needed to make the test pass. The version number
to apply when adjusting the dumps is easy enough to guess from the
reports.
--
Michael