Thread: 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
Attachment
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
Attachment
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
Attachment
> 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?s
>
> 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
> @@ -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?s
>> 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
>> @@ -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
Attachment
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
Attachment
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