Code review for XML patch. Instill a bit of sanity in the location of

the XmlExpr code in various lists, use a representation that has some hope
of reverse-listing correctly (though it's still a de-escaping function
shy of correctness), generally try to make it look more like Postgres
coding conventions.

2 files changed, 186 insertions, 115 deletions

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b2ff95f457..699b4a9370 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2,7 +2,7 @@
  * ruleutils.c	- Functions to convert stored expressions/querytrees
  *				back to source text
  *
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.237 2006/12/23 00:43:11 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.238 2006/12/24 00:29:19 tgl Exp $
  **********************************************************************/
 
 #include "postgres.h"
@@ -2988,8 +2988,8 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
 		case T_RowExpr:
 		case T_CoalesceExpr:
 		case T_MinMaxExpr:
-		case T_NullIfExpr:
 		case T_XmlExpr:
+		case T_NullIfExpr:
 		case T_Aggref:
 		case T_FuncExpr:
 			/* function-like: name(..) or name[..] */
@@ -3097,8 +3097,8 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
 				case T_RowExpr:	/* other separators */
 				case T_CoalesceExpr:	/* own parentheses */
 				case T_MinMaxExpr:		/* own parentheses */
-				case T_NullIfExpr:		/* other separators */
 				case T_XmlExpr:			/* own parentheses */
+				case T_NullIfExpr:		/* other separators */
 				case T_Aggref:	/* own parentheses */
 				case T_CaseExpr:		/* other separators */
 					return true;
@@ -3146,8 +3146,8 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
 				case T_RowExpr:	/* other separators */
 				case T_CoalesceExpr:	/* own parentheses */
 				case T_MinMaxExpr:		/* own parentheses */
-				case T_NullIfExpr:		/* other separators */
 				case T_XmlExpr:			/* own parentheses */
+				case T_NullIfExpr:		/* other separators */
 				case T_Aggref:	/* own parentheses */
 				case T_CaseExpr:		/* other separators */
 					return true;
@@ -3779,6 +3779,140 @@ get_rule_expr(Node *node, deparse_context *context,
 			}
 			break;
 
+		case T_XmlExpr:
+			{
+				XmlExpr *xexpr = (XmlExpr *) node;
+				bool	needcomma = false;
+				ListCell   *arg;
+				ListCell   *narg;
+				Const	   *con;
+
+				switch (xexpr->op)
+				{
+					case IS_XMLCONCAT:
+						appendStringInfoString(buf, "XMLCONCAT(");
+						break;
+					case IS_XMLELEMENT:
+						appendStringInfoString(buf, "XMLELEMENT(");
+						break;
+					case IS_XMLFOREST:
+						appendStringInfoString(buf, "XMLFOREST(");
+						break;
+					case IS_XMLPARSE:
+						appendStringInfoString(buf, "XMLPARSE(");
+						break;
+					case IS_XMLPI:
+						appendStringInfoString(buf, "XMLPI(");
+						break;
+					case IS_XMLROOT:
+						appendStringInfoString(buf, "XMLROOT(");
+						break;
+				}
+				if (xexpr->name)
+				{
+					/*
+					 * XXX need to de-escape the name
+					 */
+					appendStringInfo(buf, "NAME %s",
+									 quote_identifier(xexpr->name));
+					needcomma = true;
+				}
+				if (xexpr->named_args)
+				{
+					if (xexpr->op != IS_XMLFOREST)
+					{
+						if (needcomma)
+							appendStringInfoString(buf, ", ");
+						appendStringInfoString(buf, "XMLATTRIBUTES(");
+						needcomma = false;
+					}
+					forboth(arg, xexpr->named_args, narg, xexpr->arg_names)
+					{
+						Node 	*e = (Node *) lfirst(arg);
+						char	*argname = strVal(lfirst(narg));
+
+						if (needcomma)
+							appendStringInfoString(buf, ", ");
+						get_rule_expr((Node *) e, context, true);
+						/*
+						 * XXX need to de-escape the name
+						 */
+						appendStringInfo(buf, " AS %s",
+										 quote_identifier(argname));
+						needcomma = true;
+					}
+					if (xexpr->op != IS_XMLFOREST)
+						appendStringInfoChar(buf, ')');
+				}
+				if (xexpr->args)
+				{
+					if (needcomma)
+						appendStringInfoString(buf, ", ");
+					switch (xexpr->op)
+					{
+						case IS_XMLCONCAT:
+						case IS_XMLELEMENT:
+						case IS_XMLFOREST:
+						case IS_XMLPI:
+							/* no extra decoration needed */
+							get_rule_expr((Node *) xexpr->args, context, true);
+							break;
+						case IS_XMLPARSE:
+							Assert(list_length(xexpr->args) == 3);
+
+							con = (Const *) lsecond(xexpr->args);
+							Assert(IsA(con, Const));
+							Assert(!con->constisnull);
+							if (DatumGetBool(con->constvalue))
+								appendStringInfoString(buf, "DOCUMENT ");
+							else
+								appendStringInfoString(buf, "CONTENT ");
+
+							get_rule_expr((Node *) linitial(xexpr->args),
+										  context, true);
+
+							con = (Const *) lthird(xexpr->args);
+							Assert(IsA(con, Const));
+							Assert(!con->constisnull);
+							if (DatumGetBool(con->constvalue))
+								appendStringInfoString(buf,
+													   " PRESERVE WHITESPACE");
+							else
+								appendStringInfoString(buf,
+													   " STRIP WHITESPACE");
+							break;
+						case IS_XMLROOT:
+							Assert(list_length(xexpr->args) == 3);
+
+							get_rule_expr((Node *) linitial(xexpr->args),
+										  context, true);
+
+							appendStringInfoString(buf, ", VERSION ");
+							con = (Const *) lsecond(xexpr->args);
+							if (IsA(con, Const) &&
+								con->constisnull)
+								appendStringInfoString(buf, "NO VALUE");
+							else
+								get_rule_expr((Node *) con, context, false);
+
+							con = (Const *) lthird(xexpr->args);
+							Assert(IsA(con, Const));
+							if (con->constisnull)
+								/* suppress STANDALONE NO VALUE */ ;
+							else if (DatumGetBool(con->constvalue))
+								appendStringInfoString(buf,
+													   ", STANDALONE YES");
+							else
+								appendStringInfoString(buf,
+													   ", STANDALONE NO");
+							break;
+					}
+
+				}
+				appendStringInfoChar(buf, ')');
+			}
+			break;
+
 		case T_NullIfExpr:
 			{
 				NullIfExpr *nullifexpr = (NullIfExpr *) node;
@@ -3849,28 +3983,6 @@ get_rule_expr(Node *node, deparse_context *context,
 			}
 			break;
 
-		case T_XmlExpr:
-			{
-				XmlExpr *xexpr = (XmlExpr *) node;
-
-				switch (xexpr->op)
-				{
-					case IS_XMLCONCAT:
-						appendStringInfo(buf, "XMLCONCAT(");
-						break;
-					case IS_XMLELEMENT:
-						appendStringInfo(buf, "XMLELEMENT(");
-						break;
-					case IS_XMLFOREST:
-						appendStringInfo(buf, "XMLFOREST(");
-						break;
-				}
-				get_rule_expr((Node *) xexpr->named_args, context, true);
-				get_rule_expr((Node *) xexpr->args, context, true);
-				appendStringInfoChar(buf, ')');
-			}
-			break;
-
 		case T_CoerceToDomain:
 			{
 				CoerceToDomain *ctest = (CoerceToDomain *) node;
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index d765c8657f..a6dc1a6e5f 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/utils/adt/xml.c,v 1.2 2006/12/23 04:56:50 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/adt/xml.c,v 1.3 2006/12/24 00:29:19 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -41,15 +41,6 @@
 
 #ifdef USE_LIBXML
 
-/*
- * A couple of useful macros (similar to ones from libxml/parse.c)
- */
-#define CMP4( s, c1, c2, c3, c4 ) \
-  ( ((unsigned char *) s)[ 0 ] == c1 && ((unsigned char *) s)[ 1 ] == c2 && \
-    ((unsigned char *) s)[ 2 ] == c3 && ((unsigned char *) s)[ 3 ] == c4 )
-#define CMP5( s, c1, c2, c3, c4, c5 ) \
-  ( CMP4( s, c1, c2, c3, c4 ) && ((unsigned char *) s)[ 4 ] == c5 )
-
 #define PG_XML_DEFAULT_URI "dummy.xml"
 #define XML_ERRBUF_SIZE 200
 
@@ -177,31 +168,18 @@ xmlcomment(PG_FUNCTION_ARGS)
 
 
 Datum
-xmlparse(PG_FUNCTION_ARGS)
+texttoxml(PG_FUNCTION_ARGS)
 {
-#ifdef USE_LIBXML
-	text	   *data;
-	bool		is_document;
-	bool		preserve_whitespace;
-
-	data = PG_GETARG_TEXT_P(0);
+	text	   *data = PG_GETARG_TEXT_P(0);
 
-	if (PG_NARGS() >= 2)
-		is_document = PG_GETARG_BOOL(1);
-	else
-		is_document = false;
+	PG_RETURN_XML_P(xmlparse(data, false, true));
+}
 
-	if (PG_NARGS() >= 3)
-		preserve_whitespace = PG_GETARG_BOOL(2);
-	else
-		/*
-		 * Since the XMLPARSE grammar makes STRIP WHITESPACE the
-		 * default, this argument should really default to false.  But
-		 * until we have actually implemented whitespace stripping,
-		 * this would be annoying.
-		 */
-		preserve_whitespace = true;
 
+xmltype *
+xmlparse(text *data, bool is_document, bool preserve_whitespace)
+{
+#ifdef USE_LIBXML
 	if (!preserve_whitespace)
 		ereport(WARNING,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -213,111 +191,93 @@ xmlparse(PG_FUNCTION_ARGS)
 	 * valies defined by internal DTD are applied'.  As for external
 	 * DTDs, we try to support them too, (see SQL/XML:10.16.7.e)
 	 */
-	xml_parse(data, XML_PARSE_DTDATTR, is_document); /* assume that ERROR occurred if parsing failed */
+	xml_parse(data, XML_PARSE_DTDATTR, is_document);
 
-	PG_RETURN_XML_P(data);
+	return (xmltype *) data;
 #else
 	NO_XML_SUPPORT();
-	return 0;
+	return NULL;
 #endif
 }
 
 
-Datum
-xmlpi(PG_FUNCTION_ARGS)
+xmltype *
+xmlpi(char *target, text *arg)
 {
 #ifdef USE_LIBXML
-	char	   *target = NameStr(*PG_GETARG_NAME(0));
+	xmltype *result;
 	StringInfoData buf;
 
-	if (strlen(target) >= 3
-		&& (target[0] == 'x' || target[0] == 'X')
-		&& (target[1] == 'm' || target[1] == 'M')
-		&& (target[2] == 'l' || target[2] == 'L'))
-	{
+	if (pg_strncasecmp(target, "xml", 3) == 0)
 		ereport(ERROR,
-				(errcode(ERRCODE_SYNTAX_ERROR),
+				(errcode(ERRCODE_INVALID_XML_PROCESSING_INSTRUCTION),
 				 errmsg("invalid XML processing instruction"),
 				 errdetail("XML processing instruction target name cannot start with \"xml\".")));
-	}
 
 	initStringInfo(&buf);
 
-	appendStringInfo(&buf, "<?");
-	appendStringInfoString(&buf, map_sql_identifier_to_xml_name(target, false));
-	if (PG_NARGS() > 1)
+	appendStringInfo(&buf, "<?%s", target);
+
+	if (arg != NULL)
 	{
-		text *arg = PG_GETARG_TEXT_P(1);
 		char *string;
 
-		string = DatumGetCString(DirectFunctionCall1(textout, PointerGetDatum(arg)));
-		if (strstr(string, "?>"))
+		string = DatumGetCString(DirectFunctionCall1(textout,
+													 PointerGetDatum(arg)));
+		if (strstr(string, "?>") != NULL)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_XML_PROCESSING_INSTRUCTION),
 				 errmsg("invalid XML processing instruction"),
 				 errdetail("XML processing instruction cannot contain \"?>\".")));
 
-		appendStringInfoString(&buf, " ");
+		appendStringInfoChar(&buf, ' ');
 		appendStringInfoString(&buf, string);
+		pfree(string);
 	}
 	appendStringInfoString(&buf, "?>");
 
-	PG_RETURN_XML_P(stringinfo_to_xmltype(&buf));
+	result = stringinfo_to_xmltype(&buf);
+	pfree(buf.data);
+	return result;
 #else
 	NO_XML_SUPPORT();
-	return 0;
+	return NULL;
 #endif
 }
 
 
-Datum
-xmlroot(PG_FUNCTION_ARGS)
+xmltype *
+xmlroot(xmltype *data, text *version, int standalone)
 {
 #ifdef USE_LIBXML
-	xmltype	   *data;
-	text	   *version;
-	int			standalone;
+	xmltype *result;
 	StringInfoData buf;
 
-	if (PG_ARGISNULL(0))
-		PG_RETURN_NULL();
-	else
-		data = PG_GETARG_XML_P(0);
-
-	if (PG_ARGISNULL(1))
-		version = NULL;
-	else
-		version = PG_GETARG_TEXT_P(1);
-
-	if (PG_ARGISNULL(2))
-		standalone = 0;
-	else
-	{
-		bool tmp = PG_GETARG_BOOL(2);
-		standalone = (tmp ? 1 : -1);
-	}
+	initStringInfo(&buf);
 
 	/*
 	 * FIXME: This is probably supposed to be cleverer if there
 	 * already is an XML preamble.
 	 */
-	initStringInfo(&buf);
-
 	appendStringInfo(&buf,"<?xml");
-	if (version) {
+	if (version)
+	{
 		appendStringInfo(&buf, " version=\"");
 		appendStringInfoText(&buf, version);
 		appendStringInfo(&buf, "\"");
 	}
 	if (standalone)
-		appendStringInfo(&buf, " standalone=\"%s\"", (standalone == 1 ? "yes" : "no"));
+		appendStringInfo(&buf, " standalone=\"%s\"",
+						 (standalone == 1 ? "yes" : "no"));
 	appendStringInfo(&buf, "?>");
 	appendStringInfoText(&buf, (text *) data);
 
-	PG_RETURN_XML_P(stringinfo_to_xmltype(&buf));
+	result = stringinfo_to_xmltype(&buf);
+	pfree(buf.data);
+	return result;
 #else
 	NO_XML_SUPPORT();
-	return 0;
+	return NULL;
 #endif
 }
 
@@ -456,7 +416,7 @@ xml_parse(text *data, int opts, bool is_document)
 
 	/* first, we try to parse the string as XML doc, then, as XML chunk */
 	ereport(DEBUG3, (errmsg("string to parse: %s", string)));
-	if (len > 4 && CMP5(string, '<', '?', 'x', 'm', 'l'))
+	if (len >= 5 && strncmp((char *) string, "<?xml", 5) == 0)
 	{
 		/* consider it as DOCUMENT */
 		doc = xmlCtxtReadMemory(ctxt, (char *) string, len,
@@ -918,10 +878,8 @@ map_sql_identifier_to_xml_name(char *ident, bool fully_escaped)
 			appendStringInfo(&buf, "_x003A_");
 		else if (*p == '_' && *(p+1) == 'x')
 			appendStringInfo(&buf, "_x005F_");
-		else if (fully_escaped && p == ident
-				 && ( *p == 'x' || *p == 'X')
-				 && ( *(p+1) == 'm' || *(p+1) == 'M')
-				 && ( *(p+2) == 'l' || *(p+2) == 'L'))
+		else if (fully_escaped && p == ident &&
+				 pg_strncasecmp(p, "xml", 3) == 0)
 		{
 			if (*p == 'x')
 				appendStringInfo(&buf, "_x0078_");
@@ -932,9 +890,10 @@ map_sql_identifier_to_xml_name(char *ident, bool fully_escaped)
 		{
 			pg_wchar u = sqlchar_to_unicode(p);
 
-			if (!is_valid_xml_namechar(u)
-				|| (p == ident && !is_valid_xml_namefirst(u)))
-			appendStringInfo(&buf, "_x%04X_", (unsigned int) u);
+			if ((p == ident)
+				? !is_valid_xml_namefirst(u)
+				: !is_valid_xml_namechar(u))
+				appendStringInfo(&buf, "_x%04X_", (unsigned int) u);
 			else
 				appendBinaryStringInfo(&buf, p, pg_mblen(p));
 		}