Thread: Possible typo in nodeAgg.c

Hi

In /src/backend/executor/nodeAgg.c

I found the following comment still use work mem,
Since hash_mem has been introduced, Is it more accurate to use hash_mem here ?

@@ -1827,7 +1827,7 @@ hash_agg_set_limits(double hashentrysize, double input_groups, int used_bits,
     /*
      * Don't set the limit below 3/4 of hash_mem. In that case, we are at the
      * minimum number of partitions, so we aren't going to dramatically exceed
-     * work mem anyway.
+     * hash_mem anyway.

Best regards,
houzj

On Fri, Oct 16, 2020 at 09:03:52AM +0000, Hou, Zhijie wrote:
> Hi
> 
> In /src/backend/executor/nodeAgg.c
> 
> I found the following comment still use work mem,
> Since hash_mem has been introduced, Is it more accurate to use hash_mem here ?
> 
> @@ -1827,7 +1827,7 @@ hash_agg_set_limits(double hashentrysize, double input_groups, int used_bits,
>      /*
>       * Don't set the limit below 3/4 of hash_mem. In that case, we are at the
>       * minimum number of partitions, so we aren't going to dramatically exceed
> -     * work mem anyway.
> +     * hash_mem anyway.

Can someone comment on this?  Is the text change correct?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Fri, 3 Nov 2023 at 13:49, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Fri, Oct 16, 2020 at 09:03:52AM +0000, Hou, Zhijie wrote:
> >       /*
> >        * Don't set the limit below 3/4 of hash_mem. In that case, we are at the
> >        * minimum number of partitions, so we aren't going to dramatically exceed
> > -      * work mem anyway.
> > +      * hash_mem anyway.
>
> Can someone comment on this?  Is the text change correct?

"work mem" is incorrect.  I'd prefer it if we didn't talk about
hash_mem as if it were a thing.  It's work_mem * hash_mem_multiplier.
Because of the underscore, using "hash_mem" to mean this makes it look
like we're talking about a variable by that name. Maybe it would be
better to refer to the variable name that's used to store the result
of get_hash_memory_limit(), i.e. hash_mem_limit. "the limit" should
likely use "*mem_limit" instead as there are multiple limits
mentioned.

It would also be better if this comment explained what's special about
4 * partition_mem. It seems to have nothing to do with the 3/4
mentioned in the comment.

David