Subject: Missing free_var() at end of accum_sum_final()?

From: "Joel Jacobson" <[email-masked]>
Date: Thu, 16 Feb 2023 06:59:13 +0100

Hi,

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?

/Joel

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Feb 2023 15:26:26 +0900

On Thu, Feb 16, 2023 at 06:59:13AM +0100, Joel Jacobson wrote:
> 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);
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Thu, 16 Feb 2023 13:35:54 -0800

Hi,

>     /* Remove leading/trailing zeroes */

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

---

From: Michael Paquier <[email-masked]>
Date: Fri, 17 Feb 2023 11:48:14 +0900

> there's a consistency argument here.

I don't know which final result is better.  The arguments go two ways:
1) Should numeric.c be simplified so as its memory structure with extra
pfree()s, making it more consistent with more global assumptions than
just this file?  This has the disadvantage of creating more noise in
backpatching, while increasing the risk of leaks if some of the
removed parts are allocated in a tight loop within the same context.
This makes memory management less complicated.  That's how I am
understanding your point.
2) Should the style within numeric.c be more consistent?  This is how
I am understanding this proposal.  As you quote, this makes memory
management more complicated (not convinced about that for the internal
of numerics?), while making the file more consistent.

At the end, perhaps that's not worth bothering, but 2) prevails when
it comes to the rule of making some code consistent with its
surroundings.  1) has more risks seeing how old this code is.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Fri, 17 Feb 2023 12:26:26 -0800

Hi,

> understanding your point.

It's not just simplification, it's just faster to free via context reset. I
whipped up a random query exercising numeric math a bunch:

SELECT max(a + b + '17'::numeric + c) FROM (SELECT generate_series(1::numeric, 1000::numeric)) aa(a), (SELECT generate_series(1::numeric, 100::numeric)) bb(b), (SELECT generate_series(1::numeric, 10::numeric)) cc(c);

Removing the free_var()s from numeric_add_opt_error() speeds it up from ~361ms
to ~338ms.

This code really needs some memory management overhead reduction love. Many
allocation could be avoided by having a small on-stack "buffer" that's used
unless the numeric is large.

> surroundings.  1) has more risks seeing how old this code is.

I'm a bit wary that this will trigger a stream of patches to pointlessly free
things, causing churn and slowdowns. I suspect there's other places in
numeric.c where we don't free, and there certainly are a crapton in other
functions.

Greetings,

Andres Freund

---

From: "Joel Jacobson" <[email-masked]>
Date: Sun, 19 Feb 2023 20:54:11 +0100

> Removing the free_var()s from numeric_add_opt_error() speeds it up from ~361ms

I notice numeric_add_opt_error() is extern and declared in numeric.h,
called from e.g. timestamp.c and jsonpath_exec.c. Is that a problem,
i.e. is there a risk it could be used in a for loop by some code outside Numeric?

> unless the numeric is large.

Nice idea!
Could something like the attached patch work?

/Joel

---

From: "Joel Jacobson" <[email-masked]>
Date: Sun, 19 Feb 2023 23:16:43 +0100

Hi again,

Ignore previous patch, new correct version attached, that also keeps track of if the buf-field is in use or not.

Seems like your idea gives a signifiant speed-up!
On my machine, numeric_in() is 22% faster!

I ran a benchmark with 100 tests measuring execution-time of numeric_in() with two significant digits time precision.

Benchmark:

CREATE EXTENSION pit;
SELECT count(pit.async('numeric_in','{1234,0,-1}',2)) FROM generate_series(1,100);
CALL pit.work(true);
 SELECT
    format('%s(%s)',pit.test_params.function_name, array_to_string(pit.test_params.input_values,',')),
    pit.pretty_time(pit.tests.final_result),
    count(*)
FROM pit.tests
JOIN pit.test_params USING (id)
GROUP BY 1,2
ORDER BY 1,2;

HEAD:

        format         | pretty_time | count
-----------------------+-------------+-------
 numeric_in(1234,0,-1) | 31 ns       |    34
 numeric_in(1234,0,-1) | 32 ns       |    66
(2 rows)

0002-fixed-buf.patch:

        format         | pretty_time | count
-----------------------+-------------+-------
 numeric_in(1234,0,-1) | 24 ns       |     4
 numeric_in(1234,0,-1) | 25 ns       |    71
 numeric_in(1234,0,-1) | 26 ns       |    25
(3 rows)

The benchmark results was produced with: https://github.com/joelonsql/pg-timeit

Now, the question is how big of a fixed_buf we want. I will run some more benchmarks.

/Joel

On Sun, Feb 19, 2023, at 20:54, Joel Jacobson wrote:
> Attachments:
> * 0001-fixed-buf.patch

-- 
Kind regards,

Joel