Re: Another issue with invalid XML values

On Jul20, 2011, at 17:08 , Tom Lane wrote:
> Florian Pflug <fgp(at)phlo(dot)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.
>
>> But won't me miss that error entirely if me make it re-entrant?
>
> 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

Fine with me.

> (which would be problematic for my proposal,
> since I want pg_xml_done to pfree the state including the message
> buffer).

I'm fine with having pg_xml_init() palloc the state and pg_xml_done()
pfree it, but I'm kinda curious about why you prefer that over making it
the callers responsibility and letting callers use a stack-allocated
struct if they wish to.

>> 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.

Fair enough. Are you going to do that, or do you want me to produce an
updated patch? I can do that, but probably not before the weekend.

best regards,
Florian Pflug