Excerpts from Tom Lane's message of mar jul 19 19:42:54 -0400 2011:
> Now the risk factor if we do that is that if someone misses a
> pg_xml_done call, we leave an error handler installed with a context
> argument that's probably pointing at garbage, and if someone then tries
> to use libxml without re-establishing their error handler, they've
> got problems. But they'd have problems anyway with the current form of
> the patch. We could provide some defense against this by including a
> magic identifier value in the palloc'd struct and making
> xml_errorHandler check it before doing anything dangerous. Also, we
> could make pg_xml_done warn if libxml's current context pointer is
> different from the struct passed to it, which would provide another
> means of detection that somebody had missed a cleanup call.
>
> Unless someone sees a major hole in this idea, or a better way to do it,
> I'm going to modify the patch along those lines and commit.
I don't see any holes in this idea (though I didn't look very hard), but
I was thinking that maybe it's time for this module to hook onto the
cleanup stuff for the xact error case; or at least have a check that it
has been properly cleaned up elesewhere. Maybe this can be made to work
reentrantly if there's a global var holding the current context, and it
contains a link to the next one up the stack. At least, my impression
is that the PG_TRY blocks are already messy.
BTW I'd like to know your opinion on the fact that this patch added
two new StringInfo routines defined as static in xml.c. It seems to me
that if we're going to extend some module's API we should do it properly
in its own files; otherwise we're bound to repeat the functionality
elsewhere, and lose opportunities for cleaning up some other code that
could presumably use similar functionality.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
