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

On 15.03.23 22:13, Tom Lane wrote: I wrote: It occurred to me to test v23 for memory leaks, and it had bad ones: * the "newline" node used in the CONTENT case never got freed. Making another one for each line wasn't helping, either. Oh, I did really miss that one. Thanks! Pushed with those chan

Re: Memory leak in libxml2 (was Re: [PATCH] Add pretty-printed XML output option)

> On 15 Mar 2023, at 22:38, Tom Lane wrote: > After a bit of further testing: the leak is present in libxml2 2.9.7 > which is what I have on this RHEL8 box, but it seems not to occur > in libxml2 2.10.3 (tested on Fedora 37, and I verified that Fedora > isn't carrying any relevant local patch). >

Memory leak in libxml2 (was Re: [PATCH] Add pretty-printed XML output option)

I wrote: > BTW, the libxml leak problem seems to extend to other cases too. > I tested with code like > do $$ > declare x xml; t text; > begin > x := ' encoding="utf8"?>73'; > for i in 1..1000 loop > t := xmlserialize(document x as text); > end loop; > raise notice 't = %', t; > end; > $$;

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

I wrote: > Huh, interesting. That is a legitimate pretty-fication of the input, > I suppose, but some people might think it goes beyond the charter of > "indentation". I'm okay with it personally; anyone want to object? Hearing no objections to that, I moved ahead with this. It occurred to me t

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

Jim Jones writes: > On 14.03.23 18:40, Tom Lane wrote: >> I poked at this for awhile and ran into a problem that I'm not sure >> how to solve: it misbehaves for input with embedded DOCTYPE. > The issue was the flag XML_SAVE_NO_EMPTY. It was forcing empty elements > to be serialized with start-en

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

On 14.03.23 18:40, Tom Lane wrote: Jim Jones writes: [ v22-0001-Add-pretty-printed-XML-output-option.patch ] I poked at this for awhile and ran into a problem that I'm not sure how to solve: it misbehaves for input with embedded DOCTYPE. regression=# SELECT xmlserialize(DOCUMENT '' as text in

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

Jim Jones writes: > [ v22-0001-Add-pretty-printed-XML-output-option.patch ] I poked at this for awhile and ran into a problem that I'm not sure how to solve: it misbehaves for input with embedded DOCTYPE. regression=# SELECT xmlserialize(DOCUMENT '' as text indent); xmlserialize --

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

On 09.03.23 21:21, Tom Lane wrote: Peter Smith writes: The patch v19 LGTM. Another thing that's mildly irking me is that the current factorization of this code will result in xml_parse'ing the data twice, if you have both DOCUMENT and INDENT specified. We could consider avoiding that if we me

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

On 10.03.23 15:32, Tom Lane wrote: Jim Jones writes: On 09.03.23 21:21, Tom Lane wrote: I've looked through this now, and have some minor complaints and a major one. The major one is that it doesn't work for XML that doesn't satisfy IS DOCUMENT. For example, How do you suggest the output sh

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

Jim Jones writes: > On 09.03.23 21:21, Tom Lane wrote: >> I've looked through this now, and have some minor complaints and a major >> one. The major one is that it doesn't work for XML that doesn't satisfy >> IS DOCUMENT. For example, > How do you suggest the output should look like? I'd say a

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

Thanks a lot for the review! On 09.03.23 21:21, Tom Lane wrote: I've looked through this now, and have some minor complaints and a major one. The major one is that it doesn't work for XML that doesn't satisfy IS DOCUMENT. For example, regression=# select '42'::xml is document; ?column? ---

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

Peter Smith writes: > The patch v19 LGTM. I've looked through this now, and have some minor complaints and a major one. The major one is that it doesn't work for XML that doesn't satisfy IS DOCUMENT. For example, regression=# select '42'::xml is document; ?column? -- f (1 row) reg

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

On 09.03.23 18:38, Tom Lane wrote: While reviewing this patch, I started to wonder why we don't eliminate the maintenance hassle of xml_1.out by putting in a short-circuit at the top of the test, similar to those in some other scripts: /* skip test if XML support not compiled in */ SELECT 'one':

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

While reviewing this patch, I started to wonder why we don't eliminate the maintenance hassle of xml_1.out by putting in a short-circuit at the top of the test, similar to those in some other scripts: /* skip test if XML support not compiled in */ SELECT 'one'::xml; \if :ERROR \quit \endif (and I

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

The patch v19 LGTM. - v19 applies cleanly for me - Full clean build OK - HTML docs build and render OK - The 'make check' tests all pass for me - Also cfbot reports latest patch has no errors -- http://cfbot.cputube.org/ So, I marked it a "Ready for Committer" in the CF -- https://commitfest.post

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

On 23.02.23 08:51, Peter Eisentraut wrote: In kwlist.h you have     PG_KEYWORD("indent", INDENT, UNRESERVED_KEYWORD, AS_LABEL) but you can actually make it BARE_LABEL, which is preferable. More importantly, you need to add the new keyword to the bare_label_keyword production in gram.y.  I tho

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

On 23.02.23 07:36, Jim Jones wrote: On 23.02.23 02:52, Peter Smith wrote: Here are my review comments for patch v17-0001. == src/test/regress/sql/xml.sql The blank line(s) which previously separated the xmlserialize tests from the xml IS [NOT] DOCUMENT tests are now missing... v18 adds a

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

On 23.02.23 02:52, Peter Smith wrote: Here are my review comments for patch v17-0001. == src/test/regress/sql/xml.sql The blank line(s) which previously separated the xmlserialize tests from the xml IS [NOT] DOCUMENT tests are now missing... v18 adds a new line in the xml.sql file to sepa

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

Here are my review comments for patch v17-0001. == src/test/regress/sql/xml.sql The blank line(s) which previously separated the xmlserialize tests from the xml IS [NOT] DOCUMENT tests are now missing... e.g. -- indent different encoding (returns UTF-8) SELECT xmlserialize(DOCUMENT '42' AS

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

On 22.02.23 23:45, Peter Smith wrote: src/backend/executor/execExprInterp.c 2. ExecEvalXmlExpr @@ -3829,7 +3829,8 @@ ExecEvalXmlExpr(ExprState *state, ExprEvalStep *op) { Datum*argvalue = op->d.xmlexpr.argvalue; bool*argnull = op->d.xmlexpr.argnull; - + boolindent = op->

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

Here are some review comments for patch v16-0001. == > src/backend/executor/execExprInterp.c > > 2. ExecEvalXmlExpr > > @@ -3829,7 +3829,8 @@ ExecEvalXmlExpr(ExprState *state, ExprEvalStep *op) >{ >Datum*argvalue = op->d.xmlexpr.argvalue; >bool*argnull = op->d.xmlexpr.argnu

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

On 22.02.23 08:20, Peter Smith wrote: Here are some review comments for patch v15-0001 FYI, the patch applies clean and tests OK for me. == doc/src/sgml/datatype.sgml 1. XMLSERIALIZE ( { DOCUMENT | CONTENT } value AS type [ { NO INDENT | INDENT } ] ) ~ Another/shorter way to write that s

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

On 22.02.23 08:05, Nikolay Samokhvalov wrote: But is this as expected? Shouldn't it be like this:   text   13 ? Oracle and other parsers I know also do not work well with mixed contents.[1,2] I believe libxml2's parser does not know where to put the newline, as mixed values can contain mor

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

Here are some review comments for patch v15-0001 FYI, the patch applies clean and tests OK for me. == doc/src/sgml/datatype.sgml 1. XMLSERIALIZE ( { DOCUMENT | CONTENT } value AS type [ { NO INDENT | INDENT } ] ) ~ Another/shorter way to write that syntax is like below. For me, it is easie

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

On Mon, Feb 20, 2023 at 3:06 PM Jim Jones wrote: > As suggested by Peter and Nikolay, v15 now removes the xmlformat > function from the catalog and adds the [NO] INDENT option to > xmlserialize, as described in X069.\ > Great. I'm checking this patch and it seems, indentation stops working if we

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

On 18.02.23 19:09, Peter Eisentraut wrote: On 17.02.23 23:24, Nikolay Samokhvalov wrote: My idea was to follow the SQL standard (part 14, SQL/XML); unfortunately, there is no free version, but there are drafts at http://www.wiscorp.com/SQLStandards.html

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

On 17.02.23 23:24, Nikolay Samokhvalov wrote: My idea was to follow the SQL standard (part 14, SQL/XML); unfortunately, there is no free version, but there are drafts at http://www.wiscorp.com/SQLStandards.html . ::= XMLSERIALIZE [ ] AS [ ]

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

On Fri, Feb 17, 2023 at 1:14 AM Jim Jones wrote: > After your comment I'm studying the possibility to extend the existing > xmlserialize function to add the indentation feature. If so, how do you > think it should look like? An extra parameter? e.g. > > SELECT xmlserialize(DOCUMENT '42'::XML AS t

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

On 17.02.23 01:08, Andrey Borodin wrote: On Thu, Feb 16, 2023 at 2:12 PM Jim Jones wrote: I've looked into the patch. The code looks to conform to usual expectations. One nit: this comment should have just one asterisk. + /** Thanks for reviewing! Asterisk removed in v14. And I have a dumb

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

On 16.02.23 05:38, Nikolay Samokhvalov wrote: On Thu, Feb 9, 2023 at 2:31 AM Peter Eisentraut wrote: I suggest we call it "xmlformat", which is an established term for this. Some very-very old, rusted memory told me that there was something in standard – and indeed, it seems it desc

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

On Thu, Feb 16, 2023 at 2:12 PM Jim Jones wrote: > > I'm squashing v12-0001 and v12-0002 (v13 attached). I've looked into the patch. The code looks to conform to usual expectations. One nit: this comment should have just one asterisk. + /** And I have a dumb question: is this function protected fr

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

On 16.02.23 00:13, Peter Smith wrote: Today I fetched and tried the latest v11. It is failing too, but only just. - see attached file pretty-v11-results It looks only due to a whitespace EOF issue in the xml_2.out @@ -1679,4 +1679,4 @@ -- XML format: empty string SELECT xmlformat(''); ERR

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

On 16.02.23 00:13, Peter Smith wrote: Today I fetched and tried the latest v11. It is failing too, but only just. - see attached file pretty-v11-results It looks only due to a whitespace EOF issue in the xml_2.out @@ -1679,4 +1679,4 @@ -- XML format: empty string SELECT xmlformat(''); ER

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

On 16.02.23 05:38, Nikolay Samokhvalov wrote: On Thu, Feb 9, 2023 at 2:31 AM Peter Eisentraut wrote: I suggest we call it "xmlformat", which is an established term for this. Some very-very old, rusted memory told me that there was something in standard – and indeed, it seems it desc

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

On Thu, Feb 9, 2023 at 2:31 AM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > I suggest we call it "xmlformat", which is an established term for this. > Some very-very old, rusted memory told me that there was something in standard – and indeed, it seems it described an optional F

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

On Thu, Feb 16, 2023 at 12:49 AM Jim Jones wrote: > > Accidentally left the VERBOSE settings out -- sorry! > > Now it matches the approach used in a xpath test in xml.sql, xml.out, > xml_1.out and xml_2.out > > -- Since different libxml versions emit slightly different > -- error messages, we supp

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

Alvaro Herrera writes: > Note that there's another file, xml_2.out, which does not contain the > additional part of the DETAIL message. I suspect in Peter's case it's > xml_2.out that was originally being used as a comparison basis (not > xml.out), but that one is not getting patched, and ultimat

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

Accidentally left the VERBOSE settings out -- sorry! Now it matches the approach used in a xpath test in xml.sql, xml.out, xml_1.out and xml_2.out -- Since different libxml versions emit slightly different -- error messages, we suppress the DETAIL in this test. \set VERBOSITY terse SELECT xpat

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

On 15.02.23 12:11, Alvaro Herrera wrote: Well, I don't see why we would depart from what we've been doing in the rest of the XML tests. I did see that patch and I thought it was taking the wrong approach. Fair point. v10 patches the xml.out to xml_2.out - manually removing the additional lin

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

On 2023-Feb-15, Jim Jones wrote: > Hi Álvaro, > > As my test cases were not specifically about how the error message looks > like, I thought that suppressing part of the error messages by setting > VERBOSITY terse would suffice.[1] Is this approach not recommended? Well, I don't see why we would

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

On 15.02.23 11:11, Alvaro Herrera wrote: Note that there's another file, xml_2.out, which does not contain the additional part of the DETAIL message. I suspect in Peter's case it's xml_2.out that was originally being used as a comparison basis (not xml.out), but that one is not getting patched,

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

On 2023-Feb-13, Peter Smith wrote: > Something is misbehaving. > > Using the latest HEAD, and before applying the v6 patch, 'make check' > is working OK. > > But after applying the v6 patch, some XML regression tests are failing > because the DETAIL part of the message indicating the wrong synta

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

On 15.02.23 10:06, Peter Smith wrote: Bingo!! Your v9 patch now passes all 'make check' tests for me. Nice! It also passed all tests in the patch tester. But I'll leave it to a committer to decide if this VERBOSITY toggle is the best fix. I see now other test cases in the xml.sql file that al

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

On Wed, Feb 15, 2023 at 6:10 PM Jim Jones wrote: > > On 15.02.23 02:09, Peter Smith wrote: > > With v8, in my environment, in psql I see something slightly different: > > > > > > test_pub=# SET XML OPTION CONTENT; > > SET > > test_pub=# SELECT xmlformat(''); > > ERROR: invalid XML document > > DE

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

On 15.02.23 02:09, Peter Smith wrote: With v8, in my environment, in psql I see something slightly different: test_pub=# SET XML OPTION CONTENT; SET test_pub=# SELECT xmlformat(''); ERROR: invalid XML document DETAIL: line 1: switching encoding : no input line 1: Document is empty test_pub=#

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

On Wed, Feb 15, 2023 at 11:05 AM Jim Jones wrote: > > On 14.02.23 23:45, Peter Smith wrote: > > Those results implied to me that this function code (in my environment > > anyway) is somehow introducing a side effect causing the *other* XML > > tests to fail. > > I believe I've found the issue. It

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

On 14.02.23 23:45, Peter Smith wrote: Those results implied to me that this function code (in my environment anyway) is somehow introducing a side effect causing the *other* XML tests to fail. I believe I've found the issue. It is probably related to the XML OPTION settings, as it seems to del

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

On Wed, Feb 15, 2023 at 8:55 AM Jim Jones wrote: > > On 13.02.23 13:15, Jim Jones wrote: > > diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/xml.out > /tmp/cirrus-ci-build/build/testrun/regress/regress/results/xml.out > --- /tmp/cirrus-ci-build/src/test/regress/expected/xml.out 2023-02-12

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

On 13.02.23 13:15, Jim Jones wrote: diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/xml.out /tmp/cirrus-ci-build/build/testrun/regress/regress/results/xml.out --- /tmp/cirrus-ci-build/src/test/regress/expected/xml.out 2023-02-12 09:02:57.077569000 + +++ /tmp/cirrus-ci-build/bui

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

On 13.02.23 02:15, Peter Smith wrote: Something is misbehaving. Using the latest HEAD, and before applying the v6 patch, 'make check' is working OK. But after applying the v6 patch, some XML regression tests are failing because the DETAIL part of the message indicating the wrong syntax position

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

Something is misbehaving. Using the latest HEAD, and before applying the v6 patch, 'make check' is working OK. But after applying the v6 patch, some XML regression tests are failing because the DETAIL part of the message indicating the wrong syntax position is not getting displayed. Not only for

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

On 10.02.23 02:10, Peter Smith wrote: On Thu, Feb 9, 2023 at 7:17 PM Jim Jones wrote: 1. FYI, there are some whitespace warnings applying the v5 patch Trailing whitespaces removed. The patch applies now without warnings. == src/test/regress/sql/xml.sql 2. The v5 patch was already testing

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

On Thu, Feb 9, 2023 at 7:17 PM Jim Jones 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

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

On 02.02.23 21:35, Jim Jones wrote: This small patch introduces a XML pretty print function. It basically takes advantage of the indentation feature of xmlDocDumpFormatMemory from libxml2 to format XML strings. I suggest we call it "xmlformat", which is an established term for this.

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

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 un

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

Jim Jones writes: > I see your point. If I got it right, you're suggesting the following > change in the PG_TRY(); >    PG_TRY(); >     { >    int nbytes; >    if(!doc) >        xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR, >                "could not parse th

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

On 09.02.23 02:01, Peter Smith wrote: OTOH, if you are having to check for NULL doc anyway, maybe it's just as easy only doing that up-front. Then you could quick-exit the function without calling xmlDocDumpFormatMemory etc. in the first place. For example: doc = xml_parse(arg, XMLOPTION_DOCUMEN

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

On Thu, Feb 9, 2023 at 10:42 AM Jim Jones wrote: > > On 09.02.23 00:09, Peter Smith wrote: > > I noticed the xmlFreeDoc(doc) within the PG_CATCH is guarded but the > > other xmlFreeDoc(doc) is not. As the doc is assigned outside the > > PG_TRY shouldn't those both be the same? > > Hi Peter, > > My

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

On 09.02.23 00:09, Peter Smith wrote: I noticed the xmlFreeDoc(doc) within the PG_CATCH is guarded but the other xmlFreeDoc(doc) is not. As the doc is assigned outside the PG_TRY shouldn't those both be the same? Hi Peter, My logic there was the following: if program reached that part of the

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

On Thu, Feb 9, 2023 at 7:31 AM Jim Jones wrote: > > while working on another item of the TODO list I realized that I should > be using a PG_TRY() block in he xmlDocDumpFormatMemory call. > > Fixed in v5. > I noticed the xmlFreeDoc(doc) within the PG_CATCH is guarded but the other xmlFreeDoc(doc)

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

while working on another item of the TODO list I realized that I should be using a PG_TRY() block in he xmlDocDumpFormatMemory call. Fixed in v5. Best regards, Jim From f503b25c7fd8d984d29536e78577741e5e7c5e9f Mon Sep 17 00:00:00 2001 From: Jim Jones Date: Thu, 2 Feb 2023 21:27:16 +0100 Subjec

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

On 06.02.23 17:23, Tom Lane wrote: Your patch needs to also update expected/xml_1.out to match the output the test produces when run without --with-libxml. Thanks a lot! It seems to do the trick. xml_1.out updated in v4. Best, Jim From ced9fccddc033de98709a6e93dc6530ce68149db Mon Sep 17 00:00

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

Jim Jones writes: > The cfbot on "Windows - Server 2019, VS 2019 - Meson & ninja" is failing > the regression tests with the error: > ERROR:  unsupported XML feature > DETAIL:  This functionality requires the server to be built with libxml > support. > Is there anything I can do to enable libx

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

Hi, The cfbot on "Windows - Server 2019, VS 2019 - Meson & ninja" is failing the regression tests with the error: ERROR:  unsupported XML feature DETAIL:  This functionality requires the server to be built with libxml support. Is there anything I can do to enable libxml to run my regression

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

The system somehow returns different error messages in Linux and MacOS/Windows, which was causing the cfbot to fail. SELECT xmlpretty('')::xml;   ^ -DETAIL:  line 1: chunk is not well balanced +DETAIL:  line 1: Premature end of data in tag foo line 1 Test removed in v2.

[PATCH] Add pretty-printed XML output option

Hi, This small patch introduces a XML pretty print function. It basically takes advantage of the indentation feature of xmlDocDumpFormatMemory from libxml2 to format XML strings. postgres=# SELECT xmlpretty('id="z">42');     xmlpretty -- +