Re: how to correctly react on exception in pfree function?

On Wed, Oct 12, 2022 at 10:34:29PM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > On Wed, Oct 12, 2022 at 07:24:53PM -0400, Tom Lane wrote:
> >> There are hundreds, if not thousands, of "shouldn't ever happen" elogs
> >> in Postgres. We don't make any attempt to trap any of them. Why do
> >> you think this one should be different?
>
> > Because session variables are allocated in a persistent memory context, so
> > there's a code doing something like this to implement LET variable:
>
> > [...]
> > oldctxt = MemoryContextSwitchTo(SomePersistentContext);
> > newval = palloc(...);
> > MemoryContextSwitchTo(oldctxt);
> > /* No error should happen after that point or we leak memory */
> > pfree(var->val);
> > var->val = newval;
> > return;
>
> > Any error thrown in pfree would mean leaking memory forever in that backend.
>
> I've got little sympathy for that complaint, because an error
> in pfree likely means you have worse problems than a leak.

I agree, thus the question of what should be done in such case.

> Moreover, what it most likely means is that you messed up your
> memory allocations sometime earlier; making your code more
> complicated makes that risk higher not lower.
>
> Having said that, the above is really not very good code.
> Better would look like
>
> newval = MemoryContextAlloc(SomePersistentContext, ...);
> ... fill newval ...
> oldval = var->val;
> var->val = newval;
> pfree(oldval);
>
> which leaves your data structure in a consistent state whether
> pfree fails or not (and regardless of whether it fails before or
> after recycling your chunk). That property is way more important
> than the possibility of leaking some memory.

Well, it was an over simplification to outline the reason for the question.

The real code has a dedicated function to cleanup the variable (used for other
cases, like ON TRANSACTION END RESET), and one of the callers will use that
function to implement LET so that approach isn't possible as-is. Note that for
the other callers the code is written like that so the structure is consistent
whether pfree errors out or not.

We can change the API to accept an optional new value (and the few other needed
information) when cleaning the old one, but that's adding some complication
just to deal with a possible error in pfree. So it still unclear to me what to
do here.