On Mon, May 19, 2025 at 7:11 PM Jim Jones <jim.jo...@uni-muenster.de> wrote: > Not quite yet -- unless there is an expiration date that I am not aware > of :). If we decide we don't need XMLCast on Postgres after all, I'd > suggest to delete it from the todo list on the wiki [1] - I've already > added a link to this thread there.
Yeah. Just to be clear, I can't really think of committing a patch in this area because I don't know the topic well enough. I can make some general comments on what I see as issues with this patch but serious review would really need to come from a committer who is more familiar with the XML specifications than I am. If nobody like that shows up, this proposal won't be able to advance. > and since you didn't reply, I assumed I had already addressed your > comments. But now I see it was not the case. Hmm, sorry if I wasn't clear enough. I think there needs to be more explanation of quite a few things in the patch itself. > > 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. > > I'm not entirely sure what you mean by "other options". Well, the sentence begins with "Another option". Let's say we were talking about making cookies. I could say "Another option, if you don't have butter, is to substitute Crisco." But if I do that, I should then go on to explain further: "However, if you do this, it may affect the flavor of the cookies and they may brown differently in the oven. Nevertheless, it's better than not having cookies." Your patch seemed to me to be lacking any further explanation of this kind. When we document that there are multiple options, we should try to give some context to help the user choose between them. In my cookie-based example, the additional text makes it clear why I would select the Crisco option: I might be out of butter, and something is better than nothing. In your case, it was not clear to me why someone should choose XMLCAST over options or the other way around. To be clear, I don't want you to explain it *to me*. I want you to explain it to the reader of the documentation. > > 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?". > > Says the SQL/XML:2023 standard :) > > SQL/XML:2023 (ISO/IEC 9075-14:2023) - “General Rules” of §6.7.3 (d.ii.1 > and d.ii.2): Cool. You should put that in the patch. > > + 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. > > Given that XMLCast converts values between SQL and XML and vice versa, > my rationale was that if the target type is not a text type (such as > TEXTOID, VARCHAROID, or NAMEOID), then the cast operand must be of type > xml, which makes this default: safe. > [...] > But I can see it looks unsafe. Do you have something like this in mind? > [...] > default: > elog(ERROR, "unsupported target data type for XMLCast"); > } Yes, exactly. -- Robert Haas EDB: http://www.enterprisedb.com
