Re: Lack of Sanity Checking in file 'pctcl.c' for PostgreSQL 9.4.x

Michael Paquier wrote:
> On Sat, Jun 13, 2015 at 12:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> >> Michael Paquier wrote:
> >>> By the way, your patch does not compile properly and is not in-line
> >>> with the project's code format. See the updated patch attached ;)
> >
> >> ... or the conventions for allocating memory. Why not just use palloc()?
> >
> > That's hardly the fault of the proposed patch. But yeah, it seems like
> > much the best fix here is to get rid of the malloc (and strdup) calls in
> > this code in favor of using the palloc infrastructure. Even the calls
> > that *do* have manual failure checks are not compliant with our usual
> > coding standards.
>
> Hm. Regarding the code path mentioned by Bill something like the patch
> attached is enough with a memory context for the query description.

Right. Note this no longer needs the individual pfree() when aborting,
because the MemoryContextDelete would remove the whole thing. (Don't
bother resubmitting; I have already fixed it.) Will push shortly.

> Now, perhaps we could do more efforts with prodesc as well, see for
> example compile_pltcl_function for pltcl and similarly for plperl.
> Thoughts?

Right. It would simplify the code: create a memory context child of
TopTransactionContext, then compile the function, and if successful,
then MemoryContextSetParent to some longer-lived context. When the
function is invalidated, it's sufficient to delete the context and
create a new one. Creating the context as child of
TopTransactionContext allows you to avoid an explicit
MemoryContextDelete() in the elog(ERROR) cases while compiling.

I do wonder how many users this code has ...

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services