Re: Loose ends in PG XML patch

Log Message:
-----------
Initial SQL/XML support: xml data type and initial set of functions.

Modified Files:
--------------
pgsql:
configure (r1.525 -> r1.526)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/configure.diff?r1=1.525&r2=1.526)
configure.in (r1.492 -> r1.493)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/configure.in.diff?r1=1.492&r2=1.493)
pgsql/doc/src/sgml:
datatype.sgml (r1.181 -> r1.182)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/datatype.sgml.diff?r1=1.181&r2=1.182)
func.sgml (r1.347 -> r1.348)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/func.sgml.diff?r1=1.347&r2=1.348)
installation.sgml (r1.268 -> r1.269)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/installation.sgml.diff?r1=1.268&r2=1.269)
pgsql/src/backend/executor:
execQual.c (r1.199 -> r1.200)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/execQual.c.diff?r1=1.199&r2=1.200)
pgsql/src/backend/nodes:
copyfuncs.c (r1.354 -> r1.355)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/copyfuncs.c.diff?r1=1.354&r2=1.355)
equalfuncs.c (r1.288 -> r1.289)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/equalfuncs.c.diff?r1=1.288&r2=1.289)
outfuncs.c (r1.286 -> r1.287)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/outfuncs.c.diff?r1=1.286&r2=1.287)
readfuncs.c (r1.196 -> r1.197)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/readfuncs.c.diff?r1=1.196&r2=1.197)
pgsql/src/backend/optimizer/util:
clauses.c (r1.223 -> r1.224)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/optimizer/util/clauses.c.diff?r1=1.223&r2=1.224)
pgsql/src/backend/parser:
gram.y (r2.568 -> r2.569)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/gram.y.diff?r1=2.568&r2=2.569)
keywords.c (r1.177 -> r1.178)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/keywords.c.diff?r1=1.177&r2=1.178)
parse_coerce.c (r2.147 -> r2.148)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_coerce.c.diff?r1=2.147&r2=2.148)
parse_expr.c (r1.199 -> r1.200)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_expr.c.diff?r1=1.199&r2=1.200)
parse_target.c (r1.149 -> r1.150)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_target.c.diff?r1=1.149&r2=1.150)
pgsql/src/backend/utils/adt:
Makefile (r1.60 -> r1.61)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/Makefile.diff?r1=1.60&r2=1.61)
ruleutils.c (r1.235 -> r1.236)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/ruleutils.c.diff?r1=1.235&r2=1.236)
pgsql/src/backend/utils/mb:
mbutils.c (r1.59 -> r1.60)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/mb/mbutils.c.diff?r1=1.59&r2=1.60)
pgsql/src/include:
pg_config.h.in (r1.106 -> r1.107)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/pg_config.h.in.diff?r1=1.106&r2=1.107)
pgsql/src/include/catalog:
pg_cast.h (r1.26 -> r1.27)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_cast.h.diff?r1=1.26&r2=1.27)
pg_proc.h (r1.430 -> r1.431)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_proc.h.diff?r1=1.430&r2=1.431)
pg_type.h (r1.172 -> r1.173)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_type.h.diff?r1=1.172&r2=1.173)
pgsql/src/include/nodes:
execnodes.h (r1.162 -> r1.163)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/execnodes.h.diff?r1=1.162&r2=1.163)
nodes.h (r1.188 -> r1.189)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/nodes.h.diff?r1=1.188&r2=1.189)
primnodes.h (r1.118 -> r1.119)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/primnodes.h.diff?r1=1.118&r2=1.119)
pgsql/src/include/parser:
parse_coerce.h (r1.66 -> r1.67)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/parser/parse_coerce.h.diff?r1=1.66&r2=1.67)
pgsql/src/include/utils:
errcodes.h (r1.20 -> r1.21)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/errcodes.h.diff?r1=1.20&r2=1.21)
pgsql/src/test/regress:
parallel_schedule (r1.35 -> r1.36)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/parallel_schedule.diff?r1=1.35&r2=1.36)
serial_schedule (r1.33 -> r1.34)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/serial_schedule.diff?r1=1.33&r2=1.34)
pgsql/src/test/regress/expected:
opr_sanity.out (r1.68 -> r1.69)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/opr_sanity.out.diff?r1=1.68&r2=1.69)
prepare.out (r1.11 -> r1.12)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/prepare.out.diff?r1=1.11&r2=1.12)
rules.out (r1.122 -> r1.123)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/rules.out.diff?r1=1.122&r2=1.123)
pgsql/src/test/regress/sql:
opr_sanity.sql (r1.54 -> r1.55)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/sql/opr_sanity.sql.diff?r1=1.54&r2=1.55)

Added Files:
-----------
pgsql/src/backend/utils/adt:
xml.c (r1.1)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/xml.c?rev=1.1&content-type=text/x-cvsweb-markup)
pgsql/src/include/utils:
xml.h (r1.1)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/xml.h?rev=1.1&content-type=text/x-cvsweb-markup)
pgsql/src/test/regress/expected:
xml.out (r1.1)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/xml.out?rev=1.1&content-type=text/x-cvsweb-markup)
xml_1.out (r1.1)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/xml_1.out?rev=1.1&content-type=text/x-cvsweb-markup)
pgsql/src/test/regress/sql:
xml.sql (r1.1)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/sql/xml.sql?rev=1.1&content-type=text/x-cvsweb-markup)

petere(at)postgresql(dot)org (Peter Eisentraut) writes:
> Log Message:
> -----------
> Initial SQL/XML support: xml data type and initial set of functions.

Surely this commit should have included a catversion bump.

regards, tom lane

gmake check fails:
% cat src/test/regress/regression.diffs
*** ./expected/xml_1.out Thu Dec 21 21:17:57 2006
--- ./results/xml.out Thu Dec 21 21:22:24 2006
***************
*** 70,75 ****
--- 70,80 ----
standalone yes
);
ERROR: no XML support in this installation
+ SELECT xmlserialize(content data as character varying) FROM xmltest;
+ data
+ ------
+ (0 rows)
+
-- Check mapping SQL identifier to XML name
SELECT xmlpi(name ":::_xml_abc135.%-&_");
ERROR: no XML support in this installation

======================================================================

Peter Eisentraut wrote:
> Log Message:
> -----------
> Initial SQL/XML support: xml data type and initial set of functions.
>
> Modified Files:
> --------------
> pgsql:
> configure (r1.525 -> r1.526)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/configure.diff?r1=1.525&r2=1.526)
> configure.in (r1.492 -> r1.493)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/configure.in.diff?r1=1.492&r2=1.493)
> pgsql/doc/src/sgml:
> datatype.sgml (r1.181 -> r1.182)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/datatype.sgml.diff?r1=1.181&r2=1.182)
> func.sgml (r1.347 -> r1.348)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/func.sgml.diff?r1=1.347&r2=1.348)
> installation.sgml (r1.268 -> r1.269)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/installation.sgml.diff?r1=1.268&r2=1.269)
> pgsql/src/backend/executor:
> execQual.c (r1.199 -> r1.200)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/execQual.c.diff?r1=1.199&r2=1.200)
> pgsql/src/backend/nodes:
> copyfuncs.c (r1.354 -> r1.355)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/copyfuncs.c.diff?r1=1.354&r2=1.355)
> equalfuncs.c (r1.288 -> r1.289)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/equalfuncs.c.diff?r1=1.288&r2=1.289)
> outfuncs.c (r1.286 -> r1.287)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/outfuncs.c.diff?r1=1.286&r2=1.287)
> readfuncs.c (r1.196 -> r1.197)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/nodes/readfuncs.c.diff?r1=1.196&r2=1.197)
> pgsql/src/backend/optimizer/util:
> clauses.c (r1.223 -> r1.224)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/optimizer/util/clauses.c.diff?r1=1.223&r2=1.224)
> pgsql/src/backend/parser:
> gram.y (r2.568 -> r2.569)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/gram.y.diff?r1=2.568&r2=2.569)
> keywords.c (r1.177 -> r1.178)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/keywords.c.diff?r1=1.177&r2=1.178)
> parse_coerce.c (r2.147 -> r2.148)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_coerce.c.diff?r1=2.147&r2=2.148)
> parse_expr.c (r1.199 -> r1.200)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_expr.c.diff?r1=1.199&r2=1.200)
> parse_target.c (r1.149 -> r1.150)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/parse_target.c.diff?r1=1.149&r2=1.150)
> pgsql/src/backend/utils/adt:
> Makefile (r1.60 -> r1.61)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/Makefile.diff?r1=1.60&r2=1.61)
> ruleutils.c (r1.235 -> r1.236)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/ruleutils.c.diff?r1=1.235&r2=1.236)
> pgsql/src/backend/utils/mb:
> mbutils.c (r1.59 -> r1.60)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/mb/mbutils.c.diff?r1=1.59&r2=1.60)
> pgsql/src/include:
> pg_config.h.in (r1.106 -> r1.107)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/pg_config.h.in.diff?r1=1.106&r2=1.107)
> pgsql/src/include/catalog:
> pg_cast.h (r1.26 -> r1.27)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_cast.h.diff?r1=1.26&r2=1.27)
> pg_proc.h (r1.430 -> r1.431)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_proc.h.diff?r1=1.430&r2=1.431)
> pg_type.h (r1.172 -> r1.173)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_type.h.diff?r1=1.172&r2=1.173)
> pgsql/src/include/nodes:
> execnodes.h (r1.162 -> r1.163)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/execnodes.h.diff?r1=1.162&r2=1.163)
> nodes.h (r1.188 -> r1.189)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/nodes.h.diff?r1=1.188&r2=1.189)
> primnodes.h (r1.118 -> r1.119)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/nodes/primnodes.h.diff?r1=1.118&r2=1.119)
> pgsql/src/include/parser:
> parse_coerce.h (r1.66 -> r1.67)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/parser/parse_coerce.h.diff?r1=1.66&r2=1.67)
> pgsql/src/include/utils:
> errcodes.h (r1.20 -> r1.21)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/errcodes.h.diff?r1=1.20&r2=1.21)
> pgsql/src/test/regress:
> parallel_schedule (r1.35 -> r1.36)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/parallel_schedule.diff?r1=1.35&r2=1.36)
> serial_schedule (r1.33 -> r1.34)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/serial_schedule.diff?r1=1.33&r2=1.34)
> pgsql/src/test/regress/expected:
> opr_sanity.out (r1.68 -> r1.69)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/opr_sanity.out.diff?r1=1.68&r2=1.69)
> prepare.out (r1.11 -> r1.12)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/prepare.out.diff?r1=1.11&r2=1.12)
> rules.out (r1.122 -> r1.123)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/rules.out.diff?r1=1.122&r2=1.123)
> pgsql/src/test/regress/sql:
> opr_sanity.sql (r1.54 -> r1.55)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/sql/opr_sanity.sql.diff?r1=1.54&r2=1.55)
>
> Added Files:
> -----------
> pgsql/src/backend/utils/adt:
> xml.c (r1.1)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/xml.c?rev=1.1&content-type=text/x-cvsweb-markup)
> pgsql/src/include/utils:
> xml.h (r1.1)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/xml.h?rev=1.1&content-type=text/x-cvsweb-markup)
> pgsql/src/test/regress/expected:
> xml.out (r1.1)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/xml.out?rev=1.1&content-type=text/x-cvsweb-markup)
> xml_1.out (r1.1)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/xml_1.out?rev=1.1&content-type=text/x-cvsweb-markup)
> pgsql/src/test/regress/sql:
> xml.sql (r1.1)
> (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/sql/xml.sql?rev=1.1&content-type=text/x-cvsweb-markup)
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Peter Eisentraut wrote:
> Log Message:
> -----------
> Initial SQL/XML support: xml data type and initial set of functions.

this seems to cause regression failures on all the buildfarm members
(none of them are yet building with xml support).

http://www.pgbuildfarm.org/cgi-bin/show_status.pl

Stefan

Stefan Kaltenbrunner wrote:
> Peter Eisentraut wrote:
> > Log Message:
> > -----------
> > Initial SQL/XML support: xml data type and initial set of
> > functions.
>
> this seems to cause regression failures on all the buildfarm members

Should be fixed now. I don't know why that one file was outdated.
--
Peter Eisentraut
http://developer.postgresql.org/~petere/

I cleaned up some things I didn't like in the recent XML patch, but
there are several loose ends that I lack the energy to tackle now:

* Isn't mapping XMLSERIALIZE to a cast completely wrong? Aside from
the issue already noted in the code that it won't reverse-list
correctly, this loses the DOCUMENT-vs-CONTENT flag, which I suppose
must be important.

* Shouldn't the xml type support binary I/O? Right now it is the only
standard datatype that doesn't. I have no idea whether there is an
appropriate representation besides text, but if not we could define the
binary representation to be the same as text.

* Reverse-listing of XMLELEMENT and XMLPI is currently wrong because
the name string is not "de-escaped" back to a plain SQL identifier.

* It doesn't look to me like any thought has been given to localization
in xml_ereport() --- there are a ton of strings there that won't get
translated.

* I'm also quite afraid of xml_errmsg remaining non-null when the
storage it points at has been deallocated. Since this is apparently
only intended as debug support, maybe we could compile it only in debug
builds, to reduce the probability that it will fail in production?

regards, tom lane

I wrote:
> * I'm also quite afraid of xml_errmsg remaining non-null when the
> storage it points at has been deallocated. Since this is apparently
> only intended as debug support, maybe we could compile it only in debug
> builds, to reduce the probability that it will fail in production?

Actually, there's an easier and cleaner way to do the whole thing:
replace the handwritten management of xml_errmsg with a StringInfo
buffer allocated in TopMemoryContext, so that it never goes away after
having been first created. I'll go fix that.

What I'm wondering about is why this printout is emitted as a separate
DEBUG message ... wouldn't it be better to incorporate it as the DETAIL
field of the error message?

regards, tom lane

On 12/24/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> What I'm wondering about is why this printout is emitted as a separate
> DEBUG message ... wouldn't it be better to incorporate it as the DETAIL
> field of the error message?

Surely, it would. But the thing is that I couldn't manage to format
libxml2's native messages properly..

--
Best regards,
Nikolay

"Nikolay Samokhvalov" <samokhvalov(at)gmail(dot)com> writes:
> On 12/24/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> What I'm wondering about is why this printout is emitted as a separate
>> DEBUG message ... wouldn't it be better to incorporate it as the DETAIL
>> field of the error message?

> Surely, it would. But the thing is that I couldn't manage to format
> libxml2's native messages properly..

I'm not really seeing the problem. Right now I'm getting results like
this:

regression=# \set VERBOSITY verbose
regression=# set client_min_messages TO debug1;
SET
regression=# select '<foo><zit'::xml;
DEBUG: 00000: Entity: line 1: parser error : Couldn't find end of Start Tag zit line 1
<foo><zit
^
Entity: line 1: parser error : Premature end of data in tag foo line 1
<foo><zit
^

LOCATION: xml_ereport_by_code, xml.c:685
ERROR: 2200M: could not parse XML data
DETAIL: Closing tag not found
LOCATION: xml_ereport_by_code, xml.c:857
regression=# select '<?xml><foo><zit'::xml;
DEBUG: 00000: dummy.xml:1: parser error : XML declaration allowed only at the start of the document
<?xml><foo><zit
^
dummy.xml:1: parser error : ParsePI: PI xml space expected
<?xml><foo><zit
^
dummy.xml:1: parser error : ParsePI: PI xml never end ...
<?xml><foo><zit
^
dummy.xml:1: parser error : Start tag expected, '<' not found
<?xml><foo><zit
^

LOCATION: xml_ereport, xml.c:611
ERROR: 2200M: could not parse XML data
DETAIL: Start tag expected, '<' not found.
LOCATION: xml_ereport, xml.c:641
regression=#

I claim the libxml output is just fine as-is for a DETAIL message, and
we could get rid of all the pushups being done in the two versions of
xml_ereport to generate a detail message that's really insufficiently
detailed anyway. In particular, in any respectable-size chunk of XML
I think you'd *really* want some kind of error cursor position, which
the libxml output gives you.

regards, tom lane

Am Sonntag, 24. Dezember 2006 02:44 schrieb Tom Lane:
> * Isn't mapping XMLSERIALIZE to a cast completely wrong? Aside from
> the issue already noted in the code that it won't reverse-list
> correctly, this loses the DOCUMENT-vs-CONTENT flag, which I suppose
> must be important.

It is important, but at the moment it's not so important as to not provide any
standards-conforming method to obtain a character string from XML at all.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

> * Shouldn't the xml type support binary I/O? Right now it is the only
> standard datatype that doesn't. I have no idea whether there is an
> appropriate representation besides text, but if not we could define the
> binary representation to be the same as text.

There is an effort to develop a binary xml format:

http://www.w3.org/TR/xbc-characterization/