> On 2 Jan 2026, at 23:07, Marcos Magueta <[email protected]> wrote:
>
> About the patch, let me know if you find the time to review!
I was looking to review something on commitfest and decided to look into this
patch.
Unfortunately, I cannot verify adherence to SQL standard. But I'll take it as
granted, grammar changes are minimal.
I'm not a big XML user, but definitely there are a lot of use cases. E.g. If
someone want to check whole database against new schema - this feature would be
invaluable. I can think of many different use cases. But I heard some
complaints about libxml. I'm not sure, but maybe at some point we would like to
get rid of it? [0]
The patch fails regression tests on Windows. See [1]. Regression taken from [2]
Meaningfull part is:
diff --strip-trailing-cr -U3 C:/cirrus/src/test/regress/expected/xml_1.out
C:/cirrus/build/testrun/regress/regress/results/xml.out
--- C:/cirrus/src/test/regress/expected/xml_1.out 2026-01-03 19:13:07.092850000
+0000
+++ C:/cirrus/build/testrun/regress/regress/results/xml.out 2026-01-03
19:17:23.497562500 +0000
@@ -1496,3 +1496,278 @@
LINE 1: SELECT xmltext('x'|| '<P>73</P>'::xml || .42 || true || 'j':...
^
DETAIL: This functionality requires the server to be built with libxml support.
+SELECT xmlvalidate(DOCUMENT '<person><name>John</name><age>30</age></person>'
+ ACCORDING TO XMLSCHEMA '<?xml version="1.0"?>
....
So you need to update src/test/regress/expected/xml_1.out for systems without
libxml.
You use PG_TRY(); and return from that block. I found no other cases of
returning without PG_END_TRY(), looks like it is not supported.
xmloption is document_or_content. But xmlvalidate_text_schema() always
validates as a document. IDK, maybe it's correct, or maybe it works by accident.
+#else
+ NO_XML_SUPPORT();
+ return NULL;
+#endif
This NULL is returned from bool function xmlvalidate_text_schema(). I know it's
unreachable, but let's return false or true.
Also, single-line comments worth converting to our usual C comments. The patch
could benefit from pgindent.
That's all what I could find for now. Thanks for working on this!
Best regards, Andrey Borodin.
[0] https://www.postgresql.org/message-id/flat/aUK8aBluNzMZTatU%40momjian.us
[1]
https://api.cirrus-ci.com/v1/artifact/task/5580601438240768/testrun/build/testrun/regress/regress/regression.diffs
[2] https://cirrus-ci.com/task/5580601438240768
