Re: Increase of maintenance_work_mem limit in 64-bit Windows

Tom Lane писал(а) 2025-01-24 06:13:
> v(dot)popolitov(at)postgrespro(dot)ru writes:
>> [ v2-0001-work_mem_vars-limit-increased-in-64bit-Windows.patch ]
Hi Tom,

Thank you for the interest to the patch and comments.

Comment A.
> Is there a reason not to do them all the same way? I'd suggest
> "(Size) 1024", losing the "L" which has no purpose any more.
> (And note project style is with a space after the cast.)
> I don't like use of INT64CONST here because that forces an
> unnecessarily expensive calculation in 32-bit machines.

Trying to fix 64-bit code I did not consider 32-bit code seriously,
but your comment changed my mind. I tried to cast everything to
lvalue type (it was the reason of many cast kinds). Now I changed
almost everything to (Size)1024, what is 32 and 64 bit friendly.

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)

Comment B.
> 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 looked in source for all variables overflow connected to "work_mem"
variables and fixed them. I did not want to include additional code
and I tried to keep the patch as small as possible. Source has other
locations with KB to bytes conversions, but it was not the goal of
this patch.

Comment C.
> - long sort_threshold;
> + uint64 sort_threshold;
>
> (1) Are you sure the related code doesn't need this to be signed?
> (2) Even if it can be unsigned, why not Size or size_t?
Changed to Size from uint64. It should not be signed, it is compared
with positive values later and finaly casted to uint32.

Comment D.
> - long sort_mem_bytes = sort_mem * 1024L;
> + int64 sort_mem_bytes = sort_mem * INT64CONST(1024);
>
> Wrong type surely, and even more so here:
>
> - long work_mem_bytes = work_mem * 1024L;
> + double work_mem_bytes = work_mem * INT64CONST(1024);
>
> If there's actually a reason for this scattershot approach to
> new data types, you need to explain what the plan is. I'd
> have expected a push to replace "long" with "Size", or maybe
> use size_t (or ssize_t when we need a signed type).
INT64CONST(1024) changed to (Size)1024, but I keep new types
"int64" and "double" here.
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.

Comment E.
> -tbm_create(long maxbytes, dsa_area *dsa)
> +tbm_create(double maxbytes, dsa_area *dsa)
>
> Why "double"??
>
> Also, do we need to widen the result of tbm_calculate_entries?
> I see the clamp to INT_MAX-1, but should we try to get rid of
> that? (Or if not, should we reduce its result to "int"?)
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.

New version of diff:
-tbm_create(long maxbytes, dsa_area *dsa)
+tbm_create(Size maxbytes, dsa_area *dsa)

-long
-tbm_calculate_entries(double maxbytes)
+Size
+tbm_calculate_entries(Size maxbytes)
{
- long nbuckets;
+ Size nbuckets;

Also tbm_calculate_entries() is used in assignment to "long"
variable maxentries. This variable is the part of other code,
I did not touch it, only added explicit cast to long:

- maxentries = tbm_calculate_entries(work_mem * 1024L);
+ maxentries = (long)tbm_calculate_entries(work_mem * (Size)1024);

Summary:
I did fixes in patch for A,B,C,E comments.
Comment D - I gave explanation, why I changed long to int64 and double
instead of Size, I hope it is logical and you will be satisfied.

--
Best regards,

Vladlen Popolitov.