Back-patch changes of 2009-05-13 in xml.c's memory management.

I was afraid to do this when these changes were first made, but now that
8.4 has seen some field use it should be all right to back-patch.  These
changes are really quite necessary in order to give xml.c any hope of
co-existing with loadable modules that also wish to use libxml2.

src/backend/access/transam/xact.c

@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.257.2.6 2010/01/24 21:49:39 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.257.2.7 2010/03/01 02:21:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -46,7 +46,6 @@
 #include "utils/inval.h"
 #include "utils/memutils.h"
 #include "utils/relcache.h"
-#include "utils/xml.h"
 
 
 /*
@@ -1743,7 +1742,6 @@ CommitTransaction(void)
 
 	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
-	AtEOXact_xml();
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true);
 	/* smgrcommit already done */
@@ -1976,7 +1974,6 @@ PrepareTransaction(void)
 	/* PREPARE acts the same as COMMIT as far as GUC is concerned */
 	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
-	AtEOXact_xml();
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true);
 	/* smgrcommit already done */
@@ -2122,7 +2119,6 @@ AbortTransaction(void)
 
 		AtEOXact_GUC(false, 1);
 		AtEOXact_SPI(false);
-		AtEOXact_xml();
 		AtEOXact_on_commit_actions(false);
 		AtEOXact_Namespace(false);
 		smgrabort();
@@ -3956,7 +3952,6 @@ AbortSubTransaction(void)
 
 		AtEOXact_GUC(false, s->gucNestLevel);
 		AtEOSubXact_SPI(false, s->subTransactionId);
-		AtEOXact_xml();
 		AtEOSubXact_on_commit_actions(false, s->subTransactionId,
 									  s->parent->subTransactionId);
 		AtEOSubXact_Namespace(false, s->subTransactionId,

src/backend/utils/adt/xml.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/utils/adt/xml.c,v 1.68.2.11 2009/09/04 10:49:50 heikki Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/adt/xml.c,v 1.68.2.12 2010/03/01 02:21:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -26,29 +26,22 @@
 /*
  * Notes on memory management:
  *
- * Via callbacks, libxml is told to use palloc and friends for memory
- * management, within a context that we reset at transaction end (and also at
- * subtransaction abort) to prevent memory leaks.  Resetting at transaction or
- * subtransaction abort is necessary since we might have thrown a longjmp
- * while some data structures were not linked from anywhere persistent.
- * Resetting at transaction commit might not be necessary, but seems a good
- * idea to forestall long-term leaks.
- *
  * Sometimes libxml allocates global structures in the hope that it can reuse
- * them later on.  Therefore, before resetting LibxmlContext, we must tell
+ * them later on.  This makes it impractical to change the xmlMemSetup
- * libxml to discard any global data it has.  The libxml API documentation is
+ * functions on-the-fly; that is likely to lead to trying to pfree() chunks
- * not very good about specifying this, but for now we assume that
+ * allocated with malloc() or vice versa.  Since libxml might be used by
- * xmlCleanupParser() will get rid of anything we need to worry about.
+ * loadable modules, eg libperl, our only safe choices are to change the
- *
+ * functions at postmaster/backend launch or not at all.  Since we'd rather
- * We use palloc --- which will throw a longjmp on error --- for allocation
+ * not activate libxml in sessions that might never use it, the latter choice
- * callbacks that officially should act like malloc, ie, return NULL on
+ * is the preferred one.  However, for debugging purposes it can be awfully
- * out-of-memory.  This is a bit risky since there is a chance of leaving
+ * handy to constrain libxml's allocations to be done in a specific palloc
- * persistent libxml data structures in an inconsistent partially-constructed
+ * context, where they're easy to track.  Therefore there is code here that
- * state, perhaps leading to crash in xmlCleanupParser().  However, as of
+ * can be enabled in debug builds to redirect libxml's allocations into a
- * early 2008 it is *known* that libxml can crash on out-of-memory due to
+ * special context LibxmlContext.  It's not recommended to turn this on in
- * inadequate checks for NULL returns, so this behavior seems the lesser
+ * a production build because of the possibility of bad interactions with
- * of two evils.
+ * external modules.
  */
+/* #define USE_LIBXMLCONTEXT */
 
 #include "postgres.h"
 
@@ -92,19 +85,25 @@ XmlOptionType xmloption;
 #ifdef USE_LIBXML
 
 static StringInfo xml_err_buf = NULL;
-static MemoryContext LibxmlContext = NULL;
 
-static void xml_init(void);
-static void xml_memory_init(void);
-static void xml_memory_cleanup(void);
-static void *xml_palloc(size_t size);
-static void *xml_repalloc(void *ptr, size_t size);
-static void xml_pfree(void *ptr);
-static char *xml_pstrdup(const char *string);
 static void xml_ereport(int level, int sqlcode, const char *msg);
 static void xml_errorHandler(void *ctxt, const char *msg,...);
 static void xml_ereport_by_code(int level, int sqlcode,
 					const char *msg, int errcode);
+
+#ifdef USE_LIBXMLCONTEXT
+
+static MemoryContext LibxmlContext = NULL;
+
+static void xml_memory_init(void);
+static void *xml_palloc(size_t size);
+static void *xml_repalloc(void *ptr, size_t size);
+static void xml_pfree(void *ptr);
+static char *xml_pstrdup(const char *string);
+
+#endif /* USE_LIBXMLCONTEXT */
+
+static void xml_init(void);
 static xmlChar *xml_text2xmlChar(text *in);
 static int parse_xml_decl(const xmlChar * str, size_t *lenp,
 			   xmlChar ** version, xmlChar ** encoding, int *standalone);
@@ -154,15 +153,15 @@ static void SPI_sql_row_to_xmlelement(int rownum, StringInfo result,
 #ifdef USE_LIBXML
 
 static int
-xmlChar_to_encoding(xmlChar * encoding_name)
+xmlChar_to_encoding(const xmlChar *encoding_name)
 {
-	int			encoding = pg_char_to_encoding((char *) encoding_name);
+	int			encoding = pg_char_to_encoding((const char *) encoding_name);
 
 	if (encoding < 0)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("invalid encoding name \"%s\"",
-						(char *) encoding_name)));
+						(const char *) encoding_name)));
 	return encoding;
 }
 #endif
@@ -587,8 +586,8 @@ xmlelement(XmlExprState *xmlExpr, ExprContext *econtext)
 	int			i;
 	ListCell   *arg;
 	ListCell   *narg;
-	xmlBufferPtr buf;
+	xmlBufferPtr buf = NULL;
-	xmlTextWriterPtr writer;
+	xmlTextWriterPtr writer = NULL;
 
 	/*
 	 * We first evaluate all the arguments, then start up libxml and create
@@ -635,8 +634,16 @@ xmlelement(XmlExprState *xmlExpr, ExprContext *econtext)
 	/* now safe to run libxml */
 	xml_init();
 
+	PG_TRY();
+	{
 	buf = xmlBufferCreate();
+	if (!buf)
+		xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
+					"could not allocate xmlBuffer");
 	writer = xmlNewTextWriterMemory(buf, 0);
+	if (!writer)
+		xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
+					"could not allocate xmlTextWriter");
 
 	xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
 
@@ -662,9 +669,23 @@ xmlelement(XmlExprState *xmlExpr, ExprContext *econtext)
 	}
 
 	xmlTextWriterEndElement(writer);
+
+	/* we MUST do this now to flush data out to the buffer ... */
 	xmlFreeTextWriter(writer);
+	writer = NULL;
 
 	result = xmlBuffer_to_xmltype(buf);
+	}
+	PG_CATCH();
+	{
+		if (writer)
+			xmlFreeTextWriter(writer);
+		if (buf)
+			xmlBufferFree(buf);
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+
 	xmlBufferFree(buf);
 
 	return result;
@@ -821,6 +842,7 @@ xml_is_document(xmltype *arg)
 	xmlDocPtr	doc = NULL;
 	MemoryContext ccxt = CurrentMemoryContext;
 
+	/* We want to catch ereport(INVALID_XML_DOCUMENT) and return false */
 	PG_TRY();
 	{
 		doc = xml_parse((text *) arg, XMLOPTION_DOCUMENT, true,
@@ -858,19 +880,6 @@ xml_is_document(xmltype *arg)
 }
 
 
-/*
- * xml cleanup function for transaction end.  This is also called on
- * subtransaction abort; see notes at top of file for rationale.
- */
-void
-AtEOXact_xml(void)
-{
-#ifdef USE_LIBXML
-	xml_memory_cleanup();
-#endif
-}
-
-
 #ifdef USE_LIBXML
 
 /*
@@ -908,8 +917,10 @@ xml_init(void)
 		/* Now that xml_err_buf exists, safe to call xml_errorHandler */
 		xmlSetGenericErrorFunc(NULL, xml_errorHandler);
 
+#ifdef USE_LIBXMLCONTEXT
 		/* Set up memory allocation our way, too */
 		xml_memory_init();
+#endif
 
 		/* Check library compatibility */
 		LIBXML_TEST_VERSION;
@@ -923,14 +934,13 @@ xml_init(void)
 		resetStringInfo(xml_err_buf);
 
 		/*
-		 * We re-establish the callback functions every time.  This makes it
+		 * We re-establish the error callback function every time.  This makes
-		 * safe for other subsystems (PL/Perl, say) to also use libxml with
+		 * it safe for other subsystems (PL/Perl, say) to also use libxml with
 		 * their own callbacks ... so long as they likewise set up the
-		 * callbacks on every use.	It's cheap enough to not be worth worrying
+		 * callbacks on every use. It's cheap enough to not be worth worrying
 		 * about, anyway.
 		 */
 		xmlSetGenericErrorFunc(NULL, xml_errorHandler);
-		xml_memory_init();
 	}
 }
 
@@ -1142,7 +1152,7 @@ static bool
 print_xml_decl(StringInfo buf, const xmlChar * version,
 			   pg_enc encoding, int standalone)
 {
-	xml_init();
+	xml_init();					/* why is this here? */
 
 	if ((version && strcmp((char *) version, PG_XML_DEFAULT_VERSION) != 0)
 		|| (encoding && encoding != PG_UTF8)
@@ -1181,7 +1191,10 @@ print_xml_decl(StringInfo buf, const xmlChar * version,
 /*
  * Convert a C string to XML internal representation
  *
- * TODO maybe, libxml2's xmlreader is better? (do not construct DOM,
+ * Note: it is caller's responsibility to xmlFreeDoc() the result,
+ * else a permanent memory leak will ensue!
+ *
+ * TODO maybe libxml2's xmlreader is better? (do not construct DOM,
  * yet do not use SAX - see xmlreader.c)
  */
 static xmlDocPtr
@@ -1202,13 +1215,18 @@ xml_parse(text *data, XmlOptionType xmloption_arg, bool preserve_whitespace,
 										   encoding,
 										   PG_UTF8);
 
+	/* Start up libxml and its parser (no-ops if already done) */
 	xml_init();
 	xmlInitParser();
+
 	ctxt = xmlNewParserCtxt();
 	if (ctxt == NULL)
 		xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
 					"could not allocate parser context");
 
+	/* Use a TRY block to ensure the ctxt is released */
+	PG_TRY();
+	{
 	if (xmloption_arg == XMLOPTION_DOCUMENT)
 	{
 		/*
@@ -1248,9 +1266,19 @@ xml_parse(text *data, XmlOptionType xmloption_arg, bool preserve_whitespace,
 		res_code = xmlParseBalancedChunkMemory(doc, NULL, NULL, 0,
 											   utf8string + count, NULL);
 		if (res_code != 0)
+		{
+			xmlFreeDoc(doc);
 			xml_ereport(ERROR, ERRCODE_INVALID_XML_CONTENT,
 						"invalid XML content");
+		}
 	}
+	}
+	PG_CATCH();
+	{
+		xmlFreeParserCtxt(ctxt);
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
 
 	xmlFreeParserCtxt(ctxt);
 
@@ -1275,18 +1303,16 @@ xml_text2xmlChar(text *in)
 }
 
 
+#ifdef USE_LIBXMLCONTEXT
+
 /*
- * Manage the special context used for all libxml allocations
+ * Manage the special context used for all libxml allocations (but only
+ * in special debug builds; see notes at top of file)
  */
 static void
 xml_memory_init(void)
 {
-	/*
+	/* Create memory context if not there already */
-	 * Create memory context if not there already.  We make it a child of
-	 * TopMemoryContext, even though our current policy is that it doesn't
-	 * survive past transaction end, because we want to be really really
-	 * sure it doesn't go away before we've called xmlCleanupParser().
-	 */
 	if (LibxmlContext == NULL)
 		LibxmlContext = AllocSetContextCreate(TopMemoryContext,
 											  "LibxmlContext",
@@ -1298,20 +1324,6 @@ xml_memory_init(void)
 	xmlMemSetup(xml_pfree, xml_palloc, xml_repalloc, xml_pstrdup);
 }
 
-static void
-xml_memory_cleanup(void)
-{
-	if (LibxmlContext != NULL)
-	{
-		/* Give libxml a chance to clean up dangling pointers */
-		xmlCleanupParser();
-
-		/* And flush the context */
-		MemoryContextDelete(LibxmlContext);
-		LibxmlContext = NULL;
-	}
-}
-
 /*
  * Wrappers for memory management functions
  */
@@ -1344,6 +1356,8 @@ xml_pstrdup(const char *string)
 	return MemoryContextStrdup(LibxmlContext, string);
 }
 
+#endif /* USE_LIBXMLCONTEXT */
+
 
 /*
  * Wrapper for "ereport" function for XML-related errors.  The "msg"
@@ -1773,25 +1787,48 @@ map_sql_value_to_xml_value(Datum value, Oid type)
 			case BYTEAOID:
 				{
 					bytea	   *bstr = DatumGetByteaPP(value);
-					xmlBufferPtr buf;
+					xmlBufferPtr buf = NULL;
-					xmlTextWriterPtr writer;
+					xmlTextWriterPtr writer = NULL;
 					char	   *result;
 
 					xml_init();
 
-					buf = xmlBufferCreate();
+					PG_TRY();
-					writer = xmlNewTextWriterMemory(buf, 0);
+					{
+						buf = xmlBufferCreate();
+						if (!buf)
+							xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
+										"could not allocate xmlBuffer");
+						writer = xmlNewTextWriterMemory(buf, 0);
+						if (!writer)
+							xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
+										"could not allocate xmlTextWriter");
 
-					if (xmlbinary == XMLBINARY_BASE64)
+						if (xmlbinary == XMLBINARY_BASE64)
-						xmlTextWriterWriteBase64(writer, VARDATA_ANY(bstr),
+							xmlTextWriterWriteBase64(writer, VARDATA_ANY(bstr),
-												 0, VARSIZE_ANY_EXHDR(bstr));
+													 0, VARSIZE_ANY_EXHDR(bstr));
-					else
+						else
-						xmlTextWriterWriteBinHex(writer, VARDATA_ANY(bstr),
+							xmlTextWriterWriteBinHex(writer, VARDATA_ANY(bstr),
-												 0, VARSIZE_ANY_EXHDR(bstr));
+													 0, VARSIZE_ANY_EXHDR(bstr));
+
+						/* we MUST do this now to flush data out to the buffer */
+						xmlFreeTextWriter(writer);
+						writer = NULL;
+
+						result = pstrdup((const char *) xmlBufferContent(buf));
+					}
+					PG_CATCH();
+					{
+						if (writer)
+							xmlFreeTextWriter(writer);
+						if (buf)
+							xmlBufferFree(buf);
+						PG_RE_THROW();
+					}
+					PG_END_TRY();
 
-					xmlFreeTextWriter(writer);
-					result = pstrdup((const char *) xmlBufferContent(buf));
 					xmlBufferFree(buf);
+
 					return result;
 				}
 #endif   /* USE_LIBXML */
@@ -3239,25 +3276,46 @@ SPI_sql_row_to_xmlelement(int rownum, StringInfo result, char *tablename,
 static text *
 xml_xmlnodetoxmltype(xmlNodePtr cur)
 {
-	xmlChar    *str;
 	xmltype    *result;
-	size_t		len;
-	xmlBufferPtr buf;
 
 	if (cur->type == XML_ELEMENT_NODE)
 	{
+		xmlBufferPtr buf;
+
 		buf = xmlBufferCreate();
-		xmlNodeDump(buf, NULL, cur, 0, 1);
+		PG_TRY();
-		result = xmlBuffer_to_xmltype(buf);
+		{
+			xmlNodeDump(buf, NULL, cur, 0, 1);
+			result = xmlBuffer_to_xmltype(buf);
+		}
+		PG_CATCH();
+		{
+			xmlBufferFree(buf);
+			PG_RE_THROW();
+		}
+		PG_END_TRY();
 		xmlBufferFree(buf);
 	}
 	else
 	{
+		xmlChar    *str;
+
 		str = xmlXPathCastNodeToString(cur);
-		len = strlen((char *) str);
+		PG_TRY();
-		result = (text *) palloc(len + VARHDRSZ);
+		{
-		SET_VARSIZE(result, len + VARHDRSZ);
+			size_t		len;
-		memcpy(VARDATA(result), str, len);
+
+			len = strlen((char *) str);
+			result = (text *) palloc(len + VARHDRSZ);
+			SET_VARSIZE(result, len + VARHDRSZ);
+			memcpy(VARDATA(result), str, len);
+		}
+		PG_CATCH();
+		{
+			xmlFree(str);
+			PG_RE_THROW();
+		}
+		PG_END_TRY();
 		xmlFree(str);
 	}
 
@@ -3284,11 +3342,11 @@ xpath(PG_FUNCTION_ARGS)
 	xmltype    *data = PG_GETARG_XML_P(1);
 	ArrayType  *namespaces = PG_GETARG_ARRAYTYPE_P(2);
 	ArrayBuildState *astate = NULL;
-	xmlParserCtxtPtr ctxt;
+	xmlParserCtxtPtr ctxt = NULL;
-	xmlDocPtr	doc;
+	xmlDocPtr	doc = NULL;
-	xmlXPathContextPtr xpathctx;
+	xmlXPathContextPtr xpathctx = NULL;
-	xmlXPathCompExprPtr xpathcomp;
+	xmlXPathCompExprPtr xpathcomp = NULL;
-	xmlXPathObjectPtr xpathobj;
+	xmlXPathObjectPtr xpathobj = NULL;
 	char	   *datastr;
 	int32		len;
 	int32		xpath_len;
@@ -3347,8 +3405,6 @@ xpath(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_DATA_EXCEPTION),
 				 errmsg("empty XPath expression")));
 
-	xml_init();
-
 	/* These extra chars for string and xpath_expr allow for hacks below */
 
 	string = (xmlChar *) palloc((len + 8) * sizeof(xmlChar));
@@ -3357,10 +3413,12 @@ xpath(PG_FUNCTION_ARGS)
 
 	memcpy (string, datastr, len);
 	string[len] = '\0';
-	
 
+	xml_init();
 	xmlInitParser();
 
+	PG_TRY();
+	{
 	/*
 	 * redundant XML parsing (two parsings for the same value during one
 	 * command execution are possible)
@@ -3478,10 +3536,8 @@ xpath(PG_FUNCTION_ARGS)
 
 	xpathobj = xmlXPathCompiledEval(xpathcomp, xpathctx);
 	if (xpathobj == NULL)	/* TODO: reason? */
-		ereport(ERROR,
+		xml_ereport(ERROR, ERRCODE_INTERNAL_ERROR,
-				(errmsg("could not create XPath object")));
+					"could not create XPath object");
-
-	xmlXPathFreeCompExpr(xpathcomp);
 
 	/* return empty array in cases when nothing is found */
 	if (xpathobj->nodesetval == NULL)
@@ -3502,8 +3558,25 @@ xpath(PG_FUNCTION_ARGS)
 									  CurrentMemoryContext);
 		}
 	}
+	}
+	PG_CATCH();
+	{
+		if (xpathobj)
+			xmlXPathFreeObject(xpathobj);
+		if (xpathcomp)
+			xmlXPathFreeCompExpr(xpathcomp);
+		if (xpathctx)
+			xmlXPathFreeContext(xpathctx);
+		if (doc)
+			xmlFreeDoc(doc);
+		if (ctxt)
+			xmlFreeParserCtxt(ctxt);
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
 
 	xmlXPathFreeObject(xpathobj);
+	xmlXPathFreeCompExpr(xpathcomp);
 	xmlXPathFreeContext(xpathctx);
 	xmlFreeDoc(doc);
 	xmlFreeParserCtxt(ctxt);

src/include/utils/xml.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/xml.h,v 1.23 2008/01/15 18:57:00 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/utils/xml.h,v 1.23.2.1 2010/03/01 02:21:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -75,8 +75,6 @@ extern char *map_sql_identifier_to_xml_name(char *ident, bool fully_escaped, boo
 extern char *map_xml_name_to_sql_identifier(char *name);
 extern char *map_sql_value_to_xml_value(Datum value, Oid type);
 
-extern void AtEOXact_xml(void);
-
 typedef enum
 {
 	XMLBINARY_BASE64,