Michael Paquier <michael@paquier.xyz> writes:
> I'll try to have a second look at this patch in a couple of weeks.
> For now, I have added it to the next comment fest:
> https://commitfest.postgresql.org/patch/5794/
> If somebody would like to chime in, feel free.
Thanks for taking this on --- contrib/xml2 is really a mess so far as
error handling goes. Your patch looks like an improvement, although
I do have one concern: the routines in xml.c that use an xmlerrcxt
seem to check xmlerrcxt->err_occurred pretty frequently, eg xmlStrdup
is used like this:
doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
if (doc->encoding == NULL || xmlerrcxt->err_occurred)
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate XML document");
Not sure if that's needed here.
There's more that could be looked at, if you feel like it:
xml_encode_special_chars seems to need a PG_TRY block to avoid
possibly leaking "tt". I also wonder if it's really not possible
for xmlEncodeSpecialChars to fail and return NULL. (xmltext()
doesn't believe that either, but maybe it's wrong too.)
The usage of cleanup_workspace seems quite a mess: one caller
uses a PG_TRY block to ensure it's called, but the rest don't.
I also find it confusing that pgxml_xpath returns a value that is
also referenced in the workspace and cleanup_workspace is responsible
for freeing. I wonder if there's a better way to do that.
In the end of xslt_process, I wonder why
if (resstr)
xmlFree((xmlChar *) resstr);
isn't done before the pg_xml_done call.
regards, tom lane
