public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Vladlen Popolitov <[email protected]>
Cc: David Rowley <[email protected]>
Cc: [email protected]
Subject: Re: Increase of maintenance_work_mem limit in 64-bit Windows
Date: Fri, 31 Jan 2025 14:00:49 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <1a01f0-66ec2d80-3b-68487680@27595217>
	<CAApHDvrQPz_Dsm=k=6Zo044FtKNS6_CagPkWJwS1Lw7Lqm57_w@mail.gmail.com>
	<[email protected]>
	<CAApHDvqTgZvDQoHLUB6RLTOH8QT12TZ9LTinu22Bs4GPstaf7g@mail.gmail.com>
	<[email protected]>
	<CAApHDvpVqZ=HRvOVv=1TLhH1Gs_-H+b2YTZX0-Tzgz1bbpafVw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Vladlen Popolitov <[email protected]> writes:
> Only one exceptions in src/backend/commands/vacuumparallel.c:378
> shared->dead_items_info.max_bytes = vac_work_mem * (size_t)1024;
> max_bytes declared as size_t, I did cast to the same type (I cannot
> guarranty, that size_t and Size are equivalent in all systems)

I direct your attention to c.h:

/*
 * Size
 *		Size of any memory resident object, as returned by sizeof.
 */
typedef size_t Size;

It used to be possible for them to be different types, but not
anymore.  But I take your point that using "size_t" instead
makes sense if that's how the target variable is declared.
(And I feel no need to replace size_t with Size or vice versa
in code that's already correct.)

>> I wonder if it'd be worth creating a macro rather than repeating
>> "* (Size) 1024" everywhere.  KILOBYTES_TO_BYTES(work_mem) seems
>> too wordy, but maybe we can think of a shorter name.

I agree that this wasn't such a great idea, mainly because we did
not end up using exactly "Size" everywhere.

> 1) sort_mem_bytes is used as int64 variable later, passed to
> function with int64 parameter. "Size" type is not used in
> this case.
> 2) work_mem_bytes is compared only with double values later, it
> does not need additional casts in this case. "Size" type is not
> used in this case.

Agreed on these.

> Agree. It was "double" due to usage of this variable as parameter
> in tbm_calculate_entries(double maxbytes), but really
> tbm_calculate_entries() does not need this type, it again
> converted to "long" local variable. I changed parameter,
> local variable and return value of tbm_calculate_entries()
> to Size.

Agreed on the parameter being Size, but I made the return type
be int to emphasize that we're restricting the result to
integer range.  (It looks like additional work would be needed
to let it exceed INT_MAX; if anyone ever feels like doing that
work, they can change the result type again.)

> Also tbm_calculate_entries() is used in assignment to "long"
> variable maxentries.

I concluded there that we should make maxentries "double",
on the same reasoning as you have above: it's only used in
calculations with other double variables.

Pushed with some minor additional cleanups.  Many thanks for
working on this!  It was a TODO for ages.

			regards, tom lane

view thread (7+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Increase of maintenance_work_mem limit in 64-bit Windows
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox