MIME-Version: 1.0
References: 
 <CAN3aFCdx8AapWSVpJ1kaC7OC_v7QwbjgbGw9WfPBBY2GMyOadQ@mail.gmail.com>
 <CALdSSPjxLU+zhWx+CgwN+VHoHTso33trY6mse1A6Jks7hWAdrA@mail.gmail.com>
In-Reply-To: 
 <CALdSSPjxLU+zhWx+CgwN+VHoHTso33trY6mse1A6Jks7hWAdrA@mail.gmail.com>
From: Marcos Magueta <maguetamarcos@gmail.com>
Date: Sun, 7 Dec 2025 16:43:40 -0300
Message-ID: 
 <CAN3aFCesNDiL-iZg4imC0n+NgT3JywqZYkuGH83u8ssLjJ-p5Q@mail.gmail.com>
Subject: Re: WIP - xmlvalidate implementation from TODO list
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="000000000000e4c2a3064561e575"
Archived-At: 
 <https://www.postgresql.org/message-id/CAN3aFCesNDiL-iZg4imC0n%2BNgT3JywqZYkuGH83u8ssLjJ-p5Q%40mail.gmail.com>
Precedence: bulk

--000000000000e4c2a3064561e575
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thank you for your kind review!

Before I continue with the implementation, I would like to address
your concerns and discuss it further and see if it's worth carrying
on.

1) Will do!

2) The issue is the following:
```
./../../src/test/regress/pg_regress --temp-instance=3D./tmp_check
--inputdir=3D. --bindir=3D     --dlpath=3D. --max-concurrent-tests=3D20
 --schedule=3D./parallel_schedule
# +++ regress check in src/test/regress +++
# initializing database system by copying initdb template
# could not exec "sh": No such file or directory
Bail out!# postmaster failed, examine
```

This is likely due to my setup on Nix. If any command assumes paths on
conventional *nix I am often in trouble. I am checking that with a
friend, but any insights are welcome. The out file I generated was
while being completely blindfolded.

By the way, the diff you sent is assuming global paths for some
reason, so I couldn't apply it without manually changing them.

3) From what I understand, that refers to ISO/IEC 9075-14:2016, chapter and
section 11.6, page 245:
 - Parts of the grammar that reference an URI: <XML valid according to
what> ::=3D <XML valid according to URI> | <XML valid according to identifi=
er>
 - NO NAMESPACE, which unspecifies the qualification but can access
something through a LOCATI ON
 - ID which should allow access to a registered schema
So they amount to:
  ACCORDING TO XMLSCHEMA URI <uri> [LOCATION <uri>]
  ACCORDING TO XMLSCHEMA NO NAMESPACE [LOCATION <uri>]
  ACCORDING TO XMLSCHEMA ID <registered_schema_name>

What I did is rely on the protection mechanisms that are already
implemented to just side-step the issue of arbitrary retrieval.

`PgXmlErrorContext * pg_xml_init(PgXmlStrictness strictness)` starts
the xml error context preventing any attempt by libxml2 to load an
external entity (DTD, XSD from URL, local file, etc.) returns an empty
string instead. Check around the line 1420 of xml.c:

```
/*
 * Also, install an entity loader to prevent unwanted fetches of external
 * files and URLs.
 */
errcxt->saved_entityfunc =3D xmlGetExternalEntityLoader();
xmlSetExternalEntityLoader(xmlPgEntityLoader);
```

So since I am relying on a TEXT for the schema, there should be no
issues of that sort. It does however cut part of the grammar that
handles locations, which is part of the standard, and would require
this feature to be much bigger in scope...

4) I suppose you refer to doc/src/sgml/func/func-xml.sgml. Will do

5) Hmm my intent was to simply handle TEXT on the xmlschema portion,
so the expr rule on that side is indeed an oversight. Now about the
first argument, that is just following the pattern already specified
in other xml functions, like XMLPARSE and XMLSERIALIZE, which have the
same <XML Value Expression> specified in the grammar. So that might
have alraedy diverged from the standard a while back...

This is currently grammatically valid, for example:
```
select xmlparse(DOCUMENT  (select oid from pg_class limit 1));
ERROR:  invalid XML document
DETAIL:  line 1: Start tag expected, '<' not found
2619
^
```

As a summary, it does not fully implement schemas as first-class
objects, as that would require extra parts of the grammar specified in
the standard, so I capitulated to use schemas as provided text. That's
already safe in my understanding given the shielding in place. If we
are to implement the rest, I think other serious concerns would arise,
like role management, how to store schemas, etc. And when it comes
to that, is it worth all the trouble just for xml? I would like this
feature and I think the solution of relying on text is decent, since
the cost of complying 100% seems very high for low returns.

Em dom., 7 de dez. de 2025 =C3=A0s 04:34, Kirill Reshke <reshkekirill@gmail=
.com>
escreveu:

> On Sun, 7 Dec 2025 at 04:38, Marcos Magueta <maguetamarcos@gmail.com>
> wrote:
> >
> > Hello!
> >
> > I am likely one of the few people still using XML, but I noticed XSD
> schema validation is still a TODO on postgresql, which I have some person=
al
> use cases for.
> >
> > In this patch I attempt to implement XMLVALIDATE with the already in us=
e
> libxml following a version I got my hands on of the SQL/XML standard of
> 2016.
> >
> > In short, I had to add the ACCORDING word to comply with it and
> completely ignored the version that was already in the src that fetches
> arbitrary schemas (it refers to validations of dtds, which is another mor=
e
> troublesome TODO).
> >
> > I had problems running the regression tests on my machine, so I could
> only test the feature by spawning a modified instance of postgresql and
> issuing queries through psql, therefore I am marking it as WIP. If anyone
> can assert the tests pass, I would be glad.
> >
> > Also, this is my first patch, so I might have not followed standard
> practices as best as I could, so please pay particular attention to that =
on
> review.
> >
> > Cheers,
> > Marcos Magueta.
>
> HI!
>
> 1)
> > + // Default case since nothing got returned
> > + // out of the normal path for validation calls to libxml
>
> PostgreSQL uses /**/ comments style.
>
> 2)
> XML regression test suite fails, see attached. By the way, what are
> your issues with running `make check` ?
>
> 3)
> By the way, in [0] we have this
>
> `
> The function in PostgreSQL produces an =E2=80=9Cunimplemented=E2=80=9D er=
ror, because
> PostgreSQL does not have any implementation of the mechanism assumed
> in the standard for registering schemas in advance, which is necessary
> to address the security implications of a function that could refer to
> schemas from arbitrary locations.
> `
>
> How does your patch resolve this? I did not find any change in this area
>
> 4)
> Also I want to mention that without a doc, the patch is not in a
> commitable shape.
>
> 5) I am a bit surprised by this grammar rule:
>
> >  XMLVALIDATE '(' document_or_content a_expr ACCORDING TO XMLSCHEMA
> a_expr ')'
>
> this allow a wide class of expressions accepted by parser, like
>
> `SELECT xmlvalidate(DOCUMENT  (select oid from pg_class) ACCORDING TO
> XMLSCHEMA (select 32)) AS is_valid FROM xml_validation_test;`
>
> Is this expected? a_expr is way more than string constants and column
> references..  If yes, the regression test that you added, does not
> test this..
>
>
> p.s. I failed to find in google SQL/XML standard of 2016. So, I cannot
> double-check if this feature is compliant with it...
>
> [0] https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards
>
> --
> Best regards,
> Kirill Reshke
>

--000000000000e4c2a3064561e575
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Thank you for your kind review!<br><br>Before I continue w=
ith the implementation, I would like to address<br>your concerns and discus=
s it further and see if it&#39;s worth carrying<br>on.<br><br>1) Will do!<b=
r><br>2) The issue is the following:<br>```<br>./../../src/test/regress/pg_=
regress --temp-instance=3D./tmp_check --inputdir=3D. --bindir=3D =C2=A0 =C2=
=A0 --dlpath=3D. --max-concurrent-tests=3D20 =C2=A0--schedule=3D./parallel_=
schedule =C2=A0<br># +++ regress check in src/test/regress +++<br># initial=
izing database system by copying initdb template<br># could not exec &quot;=
sh&quot;: No such file or directory<br>Bail out!# postmaster failed, examin=
e<br>```<br><br>This is likely due to my setup on Nix. If any command assum=
es paths on<br>conventional *nix I am often in trouble. I am checking that =
with a<br>friend, but any insights are welcome. The out file I generated wa=
s<br>while being completely blindfolded.<br><br>By the way, the diff you se=
nt is assuming global paths for some<br>reason, so I couldn&#39;t apply it =
without manually changing them.<br><br>3) From what I understand, that refe=
rs to ISO/IEC 9075-14:2016, chapter and section 11.6, page 245:<br>=C2=A0- =
Parts of the grammar that reference an URI: &lt;XML valid according to what=
&gt; ::=3D &lt;XML valid according to URI&gt; | &lt;XML valid according to =
identifier&gt;<br>=C2=A0- NO NAMESPACE, which unspecifies the qualification=
 but can access something through a LOCATI ON<br>=C2=A0- ID which should al=
low access to a registered schema<br>So they amount to:<br>=C2=A0 ACCORDING=
 TO XMLSCHEMA URI &lt;uri&gt; [LOCATION &lt;uri&gt;]<br>=C2=A0 ACCORDING TO=
 XMLSCHEMA NO NAMESPACE [LOCATION &lt;uri&gt;]<br>=C2=A0 ACCORDING TO XMLSC=
HEMA ID &lt;registered_schema_name&gt;<br><br>What I did is rely on the pro=
tection mechanisms that are already<br>implemented to just side-step the is=
sue of arbitrary retrieval.<br><br>`PgXmlErrorContext * pg_xml_init(PgXmlSt=
rictness strictness)` starts<br>the xml error context preventing any attemp=
t by libxml2 to load an<br>external entity (DTD, XSD from URL, local file, =
etc.) returns an empty<br>string instead. Check around the line 1420 of xml=
.c:<br><br>```<br>/*<br>=C2=A0* Also, install an entity loader to prevent u=
nwanted fetches of external<br>=C2=A0* files and URLs.<br>=C2=A0*/<br>errcx=
t-&gt;saved_entityfunc =3D xmlGetExternalEntityLoader();<br>xmlSetExternalE=
ntityLoader(xmlPgEntityLoader);<br>```<br><br>So since I am relying on a TE=
XT for the schema, there should be no<br>issues of that sort. It does howev=
er cut part of the grammar that<br>handles locations, which is part of the =
standard, and would require<br>this feature to be much bigger in scope...<b=
r><br>4) I suppose you refer to doc/src/sgml/func/func-xml.sgml. Will do<br=
><br>5) Hmm my intent was to simply handle TEXT on the xmlschema portion,<b=
r>so the expr rule on that side is indeed an oversight. Now about the<br>fi=
rst argument, that is just following the pattern already specified<br>in ot=
her xml functions, like XMLPARSE and XMLSERIALIZE, which have the<br>same &=
lt;XML Value Expression&gt; specified in the grammar. So that might<br>have=
 alraedy diverged from the standard a while back...<br><br>This is currentl=
y grammatically valid, for example:<br>```<br>select xmlparse(DOCUMENT =C2=
=A0(select oid from pg_class limit 1));<br>ERROR: =C2=A0invalid XML documen=
t<br>DETAIL: =C2=A0line 1: Start tag expected, &#39;&lt;&#39; not found<br>=
2619<br>^<br>```<br><br>As a summary, it does not fully implement schemas a=
s first-class<br>objects, as that would require extra parts of the grammar =
specified in<br>the standard, so I capitulated to use schemas as provided t=
ext. That&#39;s<br>already safe in my understanding given the shielding in =
place. If we<br>are to implement the rest, I think other serious concerns w=
ould arise,<br>like role management, how to store schemas, etc. And when it=
 comes<br>to that, is it worth all the trouble just for xml? I would like t=
his<br>feature and I think the solution of relying on text is decent, since=
<br>the cost of complying 100% seems very high for low returns.</div><br><d=
iv class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" class=3D"gm=
ail_attr">Em dom., 7 de dez. de 2025 =C3=A0s 04:34, Kirill Reshke &lt;<a hr=
ef=3D"mailto:reshkekirill@gmail.com">reshkekirill@gmail.com</a>&gt; escreve=
u:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0=
.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, 7 Dec=
 2025 at 04:38, Marcos Magueta &lt;<a href=3D"mailto:maguetamarcos@gmail.co=
m" target=3D"_blank">maguetamarcos@gmail.com</a>&gt; wrote:<br>
&gt;<br>
&gt; Hello!<br>
&gt;<br>
&gt; I am likely one of the few people still using XML, but I noticed XSD s=
chema validation is still a TODO on postgresql, which I have some personal =
use cases for.<br>
&gt;<br>
&gt; In this patch I attempt to implement XMLVALIDATE with the already in u=
se libxml following a version I got my hands on of the SQL/XML standard of =
2016.<br>
&gt;<br>
&gt; In short, I had to add the ACCORDING word to comply with it and comple=
tely ignored the version that was already in the src that fetches arbitrary=
 schemas (it refers to validations of dtds, which is another more troubleso=
me TODO).<br>
&gt;<br>
&gt; I had problems running the regression tests on my machine, so I could =
only test the feature by spawning a modified instance of postgresql and iss=
uing queries through psql, therefore I am marking it as WIP. If anyone can =
assert the tests pass, I would be glad.<br>
&gt;<br>
&gt; Also, this is my first patch, so I might have not followed standard pr=
actices as best as I could, so please pay particular attention to that on r=
eview.<br>
&gt;<br>
&gt; Cheers,<br>
&gt; Marcos Magueta.<br>
<br>
HI!<br>
<br>
1)<br>
&gt; + // Default case since nothing got returned<br>
&gt; + // out of the normal path for validation calls to libxml<br>
<br>
PostgreSQL uses /**/ comments style.<br>
<br>
2)<br>
XML regression test suite fails, see attached. By the way, what are<br>
your issues with running `make check` ?<br>
<br>
3)<br>
By the way, in [0] we have this<br>
<br>
`<br>
The function in PostgreSQL produces an =E2=80=9Cunimplemented=E2=80=9D erro=
r, because<br>
PostgreSQL does not have any implementation of the mechanism assumed<br>
in the standard for registering schemas in advance, which is necessary<br>
to address the security implications of a function that could refer to<br>
schemas from arbitrary locations.<br>
`<br>
<br>
How does your patch resolve this? I did not find any change in this area<br=
>
<br>
4)<br>
Also I want to mention that without a doc, the patch is not in a<br>
commitable shape.<br>
<br>
5) I am a bit surprised by this grammar rule:<br>
<br>
&gt;=C2=A0 XMLVALIDATE &#39;(&#39; document_or_content a_expr ACCORDING TO =
XMLSCHEMA a_expr &#39;)&#39;<br>
<br>
this allow a wide class of expressions accepted by parser, like<br>
<br>
`SELECT xmlvalidate(DOCUMENT=C2=A0 (select oid from pg_class) ACCORDING TO<=
br>
XMLSCHEMA (select 32)) AS is_valid FROM xml_validation_test;`<br>
<br>
Is this expected? a_expr is way more than string constants and column<br>
references..=C2=A0 If yes, the regression test that you added, does not<br>
test this..<br>
<br>
<br>
p.s. I failed to find in google SQL/XML standard of 2016. So, I cannot<br>
double-check if this feature is compliant with it...<br>
<br>
[0] <a href=3D"https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Stand=
ards" rel=3D"noreferrer" target=3D"_blank">https://wiki.postgresql.org/wiki=
/PostgreSQL_vs_SQL/XML_Standards</a><br>
<br>
-- <br>
Best regards,<br>
Kirill Reshke<br>
</blockquote></div>

--000000000000e4c2a3064561e575--