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+39-11
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