Horton

Thread

Commits

  1. Fix cross-version upgrades with XMLSERIALIZE(NO INDENT)

  2. Add missing deparsing of [NO] IDENT to XMLSERIALIZE()

  1. Missing [NO] INDENT flag in XMLSerialize backward parsing

    Jim Jones <jim.jones@uni-muenster.de> — 2025-02-20T13:27:42Z

    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

  2. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Michael Paquier <michael@paquier.xyz> — 2025-02-21T06:31:01Z

    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

  3. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Andrew Dunstan <andrew@dunslane.net> — 2025-02-21T09:36:07Z

    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

  4. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Michael Paquier <michael@paquier.xyz> — 2025-02-21T09:55:22Z

    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

  5. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Andrew Dunstan <andrew@dunslane.net> — 2025-02-21T10:46:52Z

    

> 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

  6. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Jim Jones <jim.jones@uni-muenster.de> — 2025-02-21T11:29:12Z

    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

  7. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Michael Paquier <michael@paquier.xyz> — 2025-02-21T11:38:55Z

    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

  8. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Michael Paquier <michael@paquier.xyz> — 2025-02-21T11:46:03Z

    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

  9. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Jim Jones <jim.jones@uni-muenster.de> — 2025-02-21T11:57:24Z

    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

  10. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-21T14:53:06Z

    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

  11. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Michael Paquier <michael@paquier.xyz> — 2025-02-21T23:50:03Z

    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

  12. Re: Missing [NO] INDENT flag in XMLSerialize backward parsing

    Michael Paquier <michael@paquier.xyz> — 2025-02-24T00:19:09Z

    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