Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: WIP - xmlvalidate implementation from TODO list
From: Andrey Borodin <x4mmm@yandex-team.ru>
In-Reply-To: <89DE974B-F318-4D0A-A60B-51EDE84054E2@gmail.com>
Date: Sun, 4 Jan 2026 14:46:06 +0500
Cc: Kirill Reshke <reshkekirill@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <9A074422-2308-4BD0-9FFA-0B6D70989935@yandex-team.ru>
References: <CAN3aFCdx8AapWSVpJ1kaC7OC_v7QwbjgbGw9WfPBBY2GMyOadQ@mail.gmail.com>
 <CALdSSPjxLU+zhWx+CgwN+VHoHTso33trY6mse1A6Jks7hWAdrA@mail.gmail.com>
 <CAN3aFCesNDiL-iZg4imC0n+NgT3JywqZYkuGH83u8ssLjJ-p5Q@mail.gmail.com>
 <CAN3aFCfvVgXr77o=dB_E2kSCY+EgckSQbSBdd_N9n-LauWuQLw@mail.gmail.com>
 <CAN3aFCcx_w5Ldb+SYurwd31es9hOJqLuKARQHHDOk7+5iOqBWQ@mail.gmail.com>
 <CALdSSPhFzYCp=Aa8AAboz6TQaTmjWciQGfrEJQeOOO+0pD1GGw@mail.gmail.com>
 <89DE974B-F318-4D0A-A60B-51EDE84054E2@gmail.com>
To: Marcos Magueta <maguetamarcos@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/9A074422-2308-4BD0-9FFA-0B6D70989935%40yandex-team.ru>
Precedence: bulk


> On 2 Jan 2026, at 23:07, Marcos Magueta <maguetamarcos@gmail.com> =
wrote:
>=20
> 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=3D"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=