Hi,
On 2023-02-16 15:26:26 +0900, Michael Paquier wrote:
> On Thu, Feb 16, 2023 at 06:59:13AM +0100, Joel Jacobson wrote:
> > I noticed the NumericVar's pos_var and neg_var are not free_var()'d
> > at the end of accum_sum_final().
> >
> > The potential memory leak seems small, since the function is called
> > only once per sum() per worker (and from a few more places), but
> > maybe they should be free'd anyways for correctness?
>
> Indeed, it is true that any code path of numeric.c that relies on a
> NumericVar with an allocation done in its buffer is careful enough to
> free it, except for generate_series's SRF where one step of the
> computation is done. I don't see directly why you could not do the
> following:
> @@ -11973,6 +11973,9 @@ accum_sum_final(NumericSumAccum *accum, NumericVar *result)
> /* And add them together */
> add_var(&pos_var, &neg_var, result);
>
> + free_var(&pos_var);
> + free_var(&neg_var);
> +
> /* Remove leading/trailing zeroes */
> strip_var(result);
But why do we need it? Most SQL callable functions don't need to be careful
about not leaking O(1) memory, the exception being functions backing btree
opclasses.
In fact, the detailed memory management often is *more* expensive than just
relying on the calling memory context being reset.
Of course, numeric.c doesn't really seem to have gotten that message, so
there's a consistency argument here.
Greetings,
Andres Freund