Re: [bug fix] Memory leak in dblink

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>> Is there a need to free memory context in PG_CATCH()?
>>> In error path the memory due to this temporary context will get
>>> freed as it will delete the transaction context and this
>>> temporary context will definitely be in the hierarchy of it, so
>>> it should also get deleted. I think such handling can be
>>> useful incase we use PG_CATCH() to suppress the error.

>> Using PG_CATCH() to suppress an error is pretty much categorically unsafe.

> In some cases like for handling exceptions in plpgsql, PG_CATCH()
> is used to handle the exception and then take an appropriate action
> based on exception, so in some such cases it might be right to free
> the context memory depending on situation.

Robert's point is that the only safe way to suppress an error is to
do a (sub)transaction rollback. That will take care of cleaning up
appropriate memory contexts, along with much else. I don't see the
value of adding any single-purpose cleanups when they'd just be
subsumed by the transaction rollback anyhow.

The reason there's a PG_CATCH block here at all is to clean up libpq
PGresults that the transaction rollback logic won't be aware of.
We could instead have built infrastructure to allow rollback to clean
those up, but it'd be a lot more code, for not a lot of benefit given
the current complexity of dblink.c. Maybe sometime in the future
it'll be worth doing it that way.

regards, tom lane