Re: [PoC] XMLCast (SQL/XML X025)

On Mon, May 19, 2025 at 9:23 AM Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> wrote:
> rebase

Hi,

Well, this patch is now more than 10 months old, and it's still the
case that nobody other than the author has said that they want this.
Is it time to give up?

I still don't think it's very clear either from the patch or from the
thread how this differs from existing facilities. As far as I can see,
there are zero comments in the patch explaining the design decisions
that it makes, and nothing in the commit message, the comments, or
even the thread itself addressing my concern from my previous review:
how is this consistent, or inconsistent, with what we do in other
similar cases, and why?

To make that more concrete, the patch says:

+ Another option to convert character strings into xml is the
function <function>xmlcast</function>,
+ which is designed to cast SQL data types into <type>xml</type>,
and vice versa.

But while it explains the behavior of this option, it does not explain
how the behavior is the same or different from other options, or why.

In the comments it says:

+ /* These data types must be converted to their ISO 8601 representations */

To me this just begs the question "says who?". I think there should be
a bunch of comments in this referencing whatever document specifies
the behavior of XMLCAST, so that someone who is good at reading
specification documents (not me) can compare the implementation with
the spec and see if they agree with the decisions that were made.

+ default:
+ *op->resvalue = PointerGetDatum(DatumGetTextP(value));
+ break;

This doesn't seem very safe at all. If we know what type OIDs we
intend this to handle, then we could list them out explicitly as is
already done for TEXTOID, VARCHAROID, etc. If we don't, then why
should we believe that it's a data type for which DatumGetTextP will
produce a non-garbage return value? Maybe there's an answer to that
question, but there's no comment spelling it out; or maybe it's
actually just broken.

--
Robert Haas
EDB: http://www.enterprisedb.com