Re: Increase of maintenance_work_mem limit in 64-bit Windows

v(dot)popolitov(at)postgrespro(dot)ru writes:
> [ v2-0001-work_mem_vars-limit-increased-in-64bit-Windows.patch ]

I took a brief look at this. I think it's generally going in the
right direction, but you seem to be all over the place on how
you are doing the casts:

+ if (metadata->nPendingPages * GIN_PAGE_FREESIZE > cleanupSize * (Size)1024L)

+ scanEntry->matchBitmap = tbm_create(work_mem * INT64CONST(1024), NULL);

+ dead_items_info->max_bytes = vac_work_mem * (size_t)1024;

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.

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.

- 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?

-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"?)

- 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).

/* upper limit for GUC variables measured in kilobytes of memory */
/* note that various places assume the byte size fits in a "long" variable */
-#if SIZEOF_SIZE_T > 4 && SIZEOF_LONG > 4
+#if SIZEOF_SIZE_T > 4
#define MAX_KILOBYTES INT_MAX
#else
#define MAX_KILOBYTES (INT_MAX / 1024)

This is pretty much the crux of the whole thing, and you didn't
fix/remove the comment you falsified.

regards, tom lane