Re: [PATCH] Add pretty-printed XML output option

On Thu, Feb 9, 2023 at 7:17 PM Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> wrote:
>
> On 09.02.23 08:23, Tom Lane wrote:
> > Um ... why are you using PG_TRY here at all? It seems like
> > you have full control of the actually likely error cases.
> > The only plausible error out of the StringInfo calls is OOM,
> > and you probably don't want to trap that at all.
>
> My intention was to catch any unexpected error from
> xmlDocDumpFormatMemory and handle it properly. But I guess you're right,
> I can control the likely error cases by checking doc and nbytes.
>
> You suggest something along these lines?
>
> xmlDocPtr doc;
> xmlChar *xmlbuf = NULL;
> text *arg = PG_GETARG_TEXT_PP(0);
> StringInfoData buf;
> int nbytes;
>
> doc = xml_parse(arg, XMLOPTION_DOCUMENT, false,
> GetDatabaseEncoding(), NULL);
>
> if(!doc)
> elog(ERROR, "could not parse the given XML document");
>
> xmlDocDumpFormatMemory(doc, &xmlbuf, &nbytes, 1);
>
> xmlFreeDoc(doc);
>
> if(!nbytes)
> elog(ERROR, "could not indent the given XML document");
>
> initStringInfo(&buf);
> appendStringInfoString(&buf, (const char *)xmlbuf);
>
> xmlFree(xmlbuf);
>
> PG_RETURN_XML_P(stringinfo_to_xmltype(&buf));
>
>
> Thanks!
>

Something like that LGTM, but here are some other minor comments.

======

1.
FYI, there are some whitespace warnings applying the v5 patch

[postgres(at)CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v5-0001-Add-pretty-printed-XML-output-option.patch
../patches_misc/v5-0001-Add-pretty-printed-XML-output-option.patch:26:
trailing whitespace.

../patches_misc/v5-0001-Add-pretty-printed-XML-output-option.patch:29:
trailing whitespace.

../patches_misc/v5-0001-Add-pretty-printed-XML-output-option.patch:33:
trailing whitespace.

../patches_misc/v5-0001-Add-pretty-printed-XML-output-option.patch:37:
trailing whitespace.

../patches_misc/v5-0001-Add-pretty-printed-XML-output-option.patch:41:
trailing whitespace.

warning: squelched 48 whitespace errors
warning: 53 lines add whitespace errors.

======
src/test/regress/sql/xml.sql

2.
The v5 patch was already testing NULL, but it might be good to add
more tests to verify the function is behaving how you want for edge
cases. For example,

+-- XML pretty print: NULL, empty string, spaces only...
SELECT xmlpretty(NULL);
SELECT xmlpretty('');
SELECT xmlpretty(' ');

~~

3.
The function is returning XML anyway, so is the '::xml' casting in
these tests necessary?

e.g.
SELECT xmlpretty(NULL)::xml; --> SELECT xmlpretty(NULL);

======
src/include/catalog/pg_proc.dat

4.

+ { oid => '4642', descr => 'Indented text from xml',
+ proname => 'xmlpretty', prorettype => 'xml',
+ proargtypes => 'xml', prosrc => 'xmlpretty' },

Spurious leading space for this new entry.

======
doc/src/sgml/func.sgml

5.
+ <foo id="x">
+ <bar id="y">
+ <var id="z">42</var>
+ </bar>
+ </foo>
+
+(1 row)
+
+]]></screen>

A spurious blank line in the example after the "(1 row)"

~~~

6.
Does this function docs belong in section 9.15.1 "Producing XML
Content"? Or (since it is not really producing content) should it be
moved to the 9.15.3 "Processing XML" section?

------
Kind Regards,
Peter Smith.
Fujitsu Australia