Re: [PATCHES] ldap: fix resource leak

Neil Conway <neilc@samurai.com> writes:
> On Sat, 2006-11-04 at 23:34 -0500, Tom Lane wrote:
>> Perhaps use a PG_TRY construct?

> At least for the existing code, this doesn't work well: the function
> exits early via ereport(LOG) and then "return STATUS_ERROR;", so AFAICS
> there isn't an easy way to simplify the existing error handling logic
> via PG_TRY.

OK, no biggie.

> Note that this is related to a more general problem: if *any* backend
> function allocates a resource that needs to be manually cleaned up, it
> probably ought to be using a PG_TRY block. Otherwise, the resource will
> be leaked on elog(ERROR). I wouldn't be surprised if various parts of
> the backend neglected to get this right.

For the most part we've tried to see to it that manual cleanup isn't
required, although I agree that ldap_unbind doesn't seem worth having a
tracking mechanism for.

> However, in this particular
> case, I didn't bother doing this, since it didn't seem likely that
> anything we're going to invoke will report errors via elog. One could
> make an argument for doing, for the sake of correctness/paranoia...

In theory one could put
    START_CRIT_SECTION();
    END_CRIT_SECTION();
around the code, as a form of "Assert(no elog here)".  Not sure that
this is actually a net win though, as a PANIC might well be considered a
worse problem than a one-time leak of some LDAP state.

            regards, tom lane