XMLNAMESPACES (was Re: Clarification of nodeToString() use cases)

Andrey Lepikhov <a(dot)lepikhov(at)postgrespro(dot)ru> writes:
> In the AQO project (Adaptive Query Optimization) [1] the nodeToString()
> function is used by the planner to convert an query parse tree into a
> string to generate a hash value [2].

Hmm. Not sure that is a bright idea; in fact, I'm pretty sure it's
a *bad* idea. This choice will result in the hash randomly varying
depending on whitespace, for instance. However ...

> In PostgreSQL v.11 call nodeToString(parse) segfaulted.

... that seems like a bad thing, because we commonly use nodeToString
(particularly pprint) to dump nodetrees for debugging purposes.

> The reason is: parse tree node for XMLNAMESPACES clause has null pointer
> in the case of DEFAULT namespace (the pointer will be initialized at
> executor on the first call).

Arguably, *that* is a bug. Or at least it requires some suspicion.

> Function _outValue() uses value->val.str[0]
> [3] without checking of value->val.str.

Indeed, because Value nodes aren't supposed to contain null strings.
I doubt this is the only place that would have a problem with that.

> I want to know, which of next options is correct:
> 1. Converting a parse tree into string with nodeToString() is illegal
> operation. We need to add a comment to the description of nodeToString().

Don't like this one too much because of the debug angle.

> 2. We can use nodeToString() for parse tree convertation. In this case
> we need to check node variable 'value->val.str' to NULL pointer (Now I
> use this approach, see attachment).

This patch is unacceptable because it turns outfuncs/readfuncs into
a non-idempotent transformation, ie a Value node would not read in
the same as it wrote out.

My immediate reaction is that somebody made a bad decision about how
to represent XMLNAMESPACES clauses. It's not quite too late to fix
that; but could you offer a concrete example that triggers this,
so it's clear what case we're talking about?

regards, tom lane