Re: XMLNAMESPACES (was Re: Clarification of nodeToString() use cases) - Mailing list pgsql-hackers

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) */