| From: | Andrey Borodin <x4mmm(at)yandex-team(dot)ru> |
|---|---|
| To: | Marcos Magueta <maguetamarcos(at)gmail(dot)com> |
| Cc: | Kirill Reshke <reshkekirill(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: WIP - xmlvalidate implementation from TODO list |
| Date: | 2026-01-04 09:46:06 |
| Message-ID: | 9A074422-2308-4BD0-9FFA-0B6D70989935@yandex-team.ru |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On 2 Jan 2026, at 23:07, Marcos Magueta <maguetamarcos(at)gmail(dot)com> 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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Jelte Fennema-Nio | 2026-01-04 10:15:41 | Re: Parallelizing startup with many databases |
| Previous Message | Chao Li | 2026-01-04 09:24:12 | Re: Fix typo in code comment |