Re: Another issue with invalid XML values

Florian Pflug <fgp@phlo.org> writes:
> On Jul20, 2011, at 01:42 , Tom Lane wrote:
>> 1. If you forget pg_xml_done in some code path, you'll find out from
>> an Assert at the next pg_xml_init, which is probably far away from where
>> the actual problem is.

> Very true. In fact, I did miss one pg_xml_done() call in the xml2
> contrib module initially, and it took me a while to locate the place
> it was missing.

> But won't me miss that error entirely if me make it re-entrant?

Yeah, I don't see any very easy way to detect a missed pg_xml_done call,
but having it be an Assert failure some time later isn't good from a
robustness standpoint.

I'm of the opinion at this point that the most reliable solution is to
have a coding convention that pg_xml_init and pg_xml_done MUST be
called in the style
pg_xml_init();PG_TRY();...PG_CATCH();{    ...    pg_xml_done();    PG_RE_THROW();}PG_END_TRY();pg_xml_done();

If we convert contrib/xml2 over to this style, we can get rid of some of
the weirder aspects of the LEGACY mode, such as allowing xml_ereport to
occur after pg_xml_done (which would be problematic for my proposal,
since I want pg_xml_done to pfree the state including the message
buffer).

> I was tempted to make all the functions in xml2/ use TRY/CATCH blocks
> like the ones in core's xml.c do. The reasons I held back was I that
> I felt these cleanups weren't part of the problem my patch was trying
> to solve.

Fair point, but contorting the error handling to avoid changing xml2/ a
bit more doesn't seem like a good decision to me.  It's not like we
aren't forcing some change on that module already.

> So we really ought to make pg_xml_done() complain if libxml's
> current error context isn't what it expects.

Right, got that coded already.
        regards, tom lane