Thread: Clarification of nodeToString() use cases
Hi, Hackers! 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]. In PostgreSQL v.11 call nodeToString(parse) segfaulted. 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). Function _outValue() uses value->val.str[0] [3] without checking of value->val.str. 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(). 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). [1] https://github.com/postgrespro/aqo [2] hash.c, line 55. [3] outfuncs.c, line 3312. -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
Attachment
Andrey Lepikhov <a.lepikhov@postgrespro.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
I wrote: > Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes: >> 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). > 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? I'd thought you were talking about the raw-parsetree representation, but after poking at it, I see that the issue is with the post-parse- analysis representation. That makes this not just a minor impediment to debugging, but an easily reachable server crash, for example regression=# CREATE VIEW bogus AS SELECT * FROM XMLTABLE(XMLNAMESPACES(DEFAULT 'http://x.y'), '/rows/row' PASSING '<rows xmlns="http://x.y"><row><a>10</a></row></rows>' COLUMNS a int PATH 'a'); server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. Aside from stored rules not working, we'd also have a problem with passing such parsetrees to parallel workers (though I'm not sure whether RangeTableFunc is considered parallelizable). And you can make it crash by turning on debug_print_parse, too. The reason why we've not heard this reported is doubtless that DEFAULT isn't really supported: if you get as far as execution, you get regression=# SELECT * FROM XMLTABLE(XMLNAMESPACES(DEFAULT 'http://x.y'), '/rows/row' PASSING '<rows xmlns="http://x.y"><row><a>10</a></row></rows>' COLUMNS a int PATH 'a'); ERROR: DEFAULT namespace is not supported So I think that (a) we ought to fix the parsetree representation, perhaps as attached, and (b) we ought to backpatch into v10 where this syntax was introduced. Normally, this would be considered a change of stored rules, forcing a catversion bump and hence non-backpatchable. However, since existing releases would crash trying to store a rule containing this construct, there can't be any stored rules out there containing it, making the incompatibility moot. The change the attached patch makes is to represent a DEFAULT namespace as a NULL list entry, rather than a T_String Value node containing a null. This approach does work for all backend/nodes/ operations, but it could be argued that it's still a crash hazard for unsuspecting code. We could do something else, like use a T_Null Value to represent DEFAULT, but I'm doubtful that that's really much better. A NULL entry has the advantage (or at least I'd consider it one) of being a certain crash rather than a probabilistic crash for any uncorrected code accessing the list item. Thoughts? regards, tom lane diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.c index abbf0e4..a9fd3fd 100644 --- a/src/backend/executor/nodeTableFuncscan.c +++ b/src/backend/executor/nodeTableFuncscan.c @@ -364,8 +364,9 @@ tfuncInitialize(TableFuncScanState *tstate, ExprContext *econtext, Datum doc) forboth(lc1, tstate->ns_uris, lc2, tstate->ns_names) { ExprState *expr = (ExprState *) lfirst(lc1); - char *ns_name = strVal(lfirst(lc2)); + Value *ns_node = (Value *) lfirst(lc2); char *ns_uri; + char *ns_name; value = ExecEvalExpr((ExprState *) expr, econtext, &isnull); if (isnull) @@ -374,6 +375,9 @@ tfuncInitialize(TableFuncScanState *tstate, ExprContext *econtext, Datum doc) errmsg("namespace URI must not be null"))); ns_uri = TextDatumGetCString(value); + /* DEFAULT is passed down to SetNamespace as NULL */ + ns_name = ns_node ? strVal(ns_node) : NULL; + routine->SetNamespace(tstate, ns_name, ns_uri); } diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index cfd4b91..d6b93f5 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -779,7 +779,7 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) /* undef ordinality column number */ tf->ordinalitycol = -1; - + /* Process column specs */ names = palloc(sizeof(char *) * list_length(rtf->columns)); colno = 0; @@ -900,15 +900,15 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) { foreach(lc2, ns_names) { - char *name = strVal(lfirst(lc2)); + Value *ns_node = (Value *) lfirst(lc2); - if (name == NULL) + if (ns_node == NULL) continue; - if (strcmp(name, r->name) == 0) + if (strcmp(strVal(ns_node), r->name) == 0) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("namespace name \"%s\" is not unique", - name), + r->name), parser_errposition(pstate, r->location))); } } @@ -922,8 +922,9 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) default_ns_seen = true; } - /* Note the string may be NULL */ - ns_names = lappend(ns_names, makeString(r->name)); + /* We represent DEFAULT by a null pointer */ + ns_names = lappend(ns_names, + r->name ? makeString(r->name) : NULL); } tf->ns_uris = ns_uris; diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 4c2408d..eecd64e 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -9739,17 +9739,17 @@ get_tablefunc(TableFunc *tf, deparse_context *context, bool showimplicit) forboth(lc1, tf->ns_uris, lc2, tf->ns_names) { Node *expr = (Node *) lfirst(lc1); - char *name = strVal(lfirst(lc2)); + Value *ns_node = (Value *) lfirst(lc2); if (!first) appendStringInfoString(buf, ", "); else first = false; - if (name != NULL) + if (ns_node != NULL) { get_rule_expr(expr, context, showimplicit); - appendStringInfo(buf, " AS %s", name); + appendStringInfo(buf, " AS %s", strVal(ns_node)); } else { diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index c830f14..687d7cd 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -1573,8 +1573,8 @@ typedef struct TableFuncScanState ExprState *rowexpr; /* state for row-generating expression */ List *colexprs; /* state for column-generating expression */ List *coldefexprs; /* state for column default expressions */ - List *ns_names; /* list of str nodes with namespace names */ - List *ns_uris; /* list of states of namespace uri exprs */ + List *ns_names; /* same as TableFunc.ns_names */ + List *ns_uris; /* list of states of namespace URI exprs */ Bitmapset *notnulls; /* nullability flag for each output column */ void *opaque; /* table builder private space */ const struct TableFuncRoutine *routine; /* table builder methods */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 1b4b0d7..40f6eb0 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -75,12 +75,15 @@ typedef struct RangeVar /* * TableFunc - node for a table function, such as XMLTABLE. + * + * Entries in the ns_names list are either string Value nodes containing + * literal namespace names, or NULL pointers to represent DEFAULT. */ typedef struct TableFunc { NodeTag type; - List *ns_uris; /* list of namespace uri */ - List *ns_names; /* list of namespace names */ + List *ns_uris; /* list of namespace URI expressions */ + List *ns_names; /* list of namespace names or NULL */ Node *docexpr; /* input document expression */ Node *rowexpr; /* row filter expression */ List *colnames; /* column names (list of String) */
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> The change the attached patch makes is to represent a DEFAULT Tom> namespace as a NULL list entry, rather than a T_String Value node Tom> containing a null. This approach does work for all backend/nodes/ Tom> operations, but it could be argued that it's still a crash hazard Tom> for unsuspecting code. We could do something else, like use a Tom> T_Null Value to represent DEFAULT, but I'm doubtful that that's Tom> really much better. A NULL entry has the advantage (or at least Tom> I'd consider it one) of being a certain crash rather than a Tom> probabilistic crash for any uncorrected code accessing the list Tom> item. Thoughts? Seems reasonable to me. Tom> + Value *ns_node = (Value *) lfirst(lc2); lfirst_node(Value, lc2) maybe? -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> + Value *ns_node = (Value *) lfirst(lc2);
> lfirst_node(Value, lc2) maybe?
Unfortunately not: the node tag is not T_Value but T_String.
regards, tom lane
On 09/16/2018 02:05 PM, Tom Lane wrote: > I wrote: >> Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes: >>> 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). >> 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? > I'd thought you were talking about the raw-parsetree representation, > but after poking at it, I see that the issue is with the post-parse- > analysis representation. That makes this not just a minor impediment > to debugging, but an easily reachable server crash, for example > > regression=# CREATE VIEW bogus AS > SELECT * FROM XMLTABLE(XMLNAMESPACES(DEFAULT 'http://x.y'), > '/rows/row' > PASSING '<rows xmlns="http://x.y"><row><a>10</a></row></rows>' COLUMNS a int PATH 'a'); > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. > > Aside from stored rules not working, we'd also have a problem with > passing such parsetrees to parallel workers (though I'm not sure > whether RangeTableFunc is considered parallelizable). And you can > make it crash by turning on debug_print_parse, too. > > The reason why we've not heard this reported is doubtless that > DEFAULT isn't really supported: if you get as far as execution, > you get > > regression=# SELECT * FROM XMLTABLE(XMLNAMESPACES(DEFAULT 'http://x.y'), > '/rows/row' > PASSING '<rows xmlns="http://x.y"><row><a>10</a></row></rows>' > COLUMNS a int PATH 'a'); > ERROR: DEFAULT namespace is not supported > > So I think that (a) we ought to fix the parsetree representation, > perhaps as attached, and (b) we ought to backpatch into v10 where this > syntax was introduced. Normally, this would be considered a change > of stored rules, forcing a catversion bump and hence non-backpatchable. > However, since existing releases would crash trying to store a rule > containing this construct, there can't be any stored rules out there > containing it, making the incompatibility moot. > > The change the attached patch makes is to represent a DEFAULT namespace > as a NULL list entry, rather than a T_String Value node containing a > null. This approach does work for all backend/nodes/ operations, but > it could be argued that it's still a crash hazard for unsuspecting code. > We could do something else, like use a T_Null Value to represent DEFAULT, > but I'm doubtful that that's really much better. A NULL entry has the > advantage (or at least I'd consider it one) of being a certain crash > rather than a probabilistic crash for any uncorrected code accessing > the list item. Thoughts? > > Seems related to this CF item that's been around for a year: <https://www.postgresql.org/message-id/flat/CAFj8pRB%2BWDyDcZyGmfRdJ0HOoXugeaL-KNFeK9YA5Z10JN9qfA%40mail.gmail.com> ? cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > On 09/16/2018 02:05 PM, Tom Lane wrote: >> The change the attached patch makes is to represent a DEFAULT namespace >> as a NULL list entry, rather than a T_String Value node containing a >> null. > Seems related to this CF item that's been around for a year: > <https://www.postgresql.org/message-id/flat/CAFj8pRB%2BWDyDcZyGmfRdJ0HOoXugeaL-KNFeK9YA5Z10JN9qfA%40mail.gmail.com> > ? Hm, seems like that is operating at the next level down; it starts with XmlTableSetNamespace's response to a null "name" argument, whereas what I'm on about is what happens before we get to that function. There's quite a bit I don't like about that patch now that I look at it :-(, but I don't think it's relevant to this thread. regards, tom lane
16.09.2018 23:05, Tom Lane writes: >> Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes: >>> 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). > >> 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? > The change the attached patch makes is to represent a DEFAULT namespace > as a NULL list entry, rather than a T_String Value node containing a > null. This approach does work for all backend/nodes/ operations, but > it could be argued that it's still a crash hazard for unsuspecting code. > We could do something else, like use a T_Null Value to represent DEFAULT, > but I'm doubtful that that's really much better. A NULL entry has the > advantage (or at least I'd consider it one) of being a certain crash > rather than a probabilistic crash for any uncorrected code accessing > the list item. Thoughts? You correctly defined the problem in more general formulation at your next thread [1]. Thank you for this patch. May be it is not universal solution for DEFAULT values, but now it works fine. Note, however, that if we emphasize comments by DEBUG purpose of nodeToString(), it can reduce the same mistakes and questions in the future. [1] More deficiencies in outfuncs/readfuncs processing: https://www.postgresql.org/message-id/17114.1537138992@sss.pgh.pa.us -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
On 09/16/2018 11:11 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 09/16/2018 02:05 PM, Tom Lane wrote: >>> The change the attached patch makes is to represent a DEFAULT namespace >>> as a NULL list entry, rather than a T_String Value node containing a >>> null. >> Seems related to this CF item that's been around for a year: >> <https://www.postgresql.org/message-id/flat/CAFj8pRB%2BWDyDcZyGmfRdJ0HOoXugeaL-KNFeK9YA5Z10JN9qfA%40mail.gmail.com> >> ? > Hm, seems like that is operating at the next level down; it starts with > XmlTableSetNamespace's response to a null "name" argument, whereas what > I'm on about is what happens before we get to that function. > > There's quite a bit I don't like about that patch now that I look at it > :-(, but I don't think it's relevant to this thread. > > OK, good. Please do add your comments to the appropriate thread and change the CF status if required. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services