Re: Commitfest Status: Sudden Death Overtime

On Jul18, 2011, at 22:19 , Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> We're down to three patches that fall into this category: two XML
>> patches by Florian, which have been somewhat derailed by an argument
>> about whether values of type xml should, in fact, be required to be
>> valid xml (I think you know my vote on this one...);
>
> Yeah, it's not clear to me either which of those are still reasonable
> candidates for committing as-is.

There are actually three XML-related patches from me in the queue.
I'll try to give an overview of their states - as perceived by me,
of course. If people want to comment on this, I suggest to do that
that either on the existing threads from these patches or on new ones,
but not on this one - lest confusion ensues.

* Bugfix for XPATH() if text or attribute nodes are selected
https://commitfest.postgresql.org/action/patch_view?id=580

Makes XPATH() return valid XML if text or attribute are selected.

I'm not aware of any issues with that one, other than Radoslaw's
unhappiness about the change of behaviour. Since the current behaviour
is very clearly broken (as in dump, reload, ka-Woom), I'm not prepared
to accept that as a show-stopper.

The question about whether values of type XML should or should
not be in fact valid XML is a red herring. That question has long
ago been settles by the XML type's input function, which has a pretty
clear and consistent idea about what constitutes a valid value of type
XML. The patch simply makes XPATH() abide by those rules, instead of
making up rules of it's own pretty nilly-willy.

* Bugfix for XPATH() if expression returns a scalar value
https://commitfest.postgresql.org/action/patch_view?id=565

Makes XPATH() support XPath expressions which compute a scalar value,
not a node set (i.e. apply a top-level function such as name()).
Currently, we return an empty array in that case.

Peter Eisentraut initially seemed to prefer creating separate functions
for that (XPATH_STRING, ...). I argued against that, on the grounds that
that'd make it impossible to evaluate user-supplied XPath expression (since
you wouldn't know which function to use). I haven't heard back from him
after that argument. Radoslaw liked the patch, but wanted the quoting
removed - same theme as with the other patch.

* XML error handling improvement to fix XPATH bug
https://commitfest.postgresql.org/action/patch_view?id=579

Like that first patch, it fixes a bug where XPATH() returns invalid
XML. The cause is completely different though. Here, libxml is (partially)
at fault - it's parsing methods always return a document instance if
a document is *structurally* valid, even if it contains semantic error
like invalid namespace references. But it isn't fully prepared to
actually handle these inconsistent documents - for example, when asked
to render a namespace URI as text, it unconditionally assumes it doesn't
have to escape it, because it may not contain special characters anway.
Which, if course, isn't necessarily true for structurally valid but
semantically invalid documents...
valid...

Fixing that wasn't possible without a major rewrite of the XML support's
error handling - one must use the modern version of libxml's error handling
infrastructure to actually be able to detect these semantic error reliably
and distinguish them from mere warnings.

Noah Misch has reviewed that patch pretty extensively (Thanks again, Noah!),
and I've resolved all his concerns regarding the implementation. I haven't
seen a single argument *against* applying this so far.

best regards,
Florian Pflug