Re: Increase of maintenance_work_mem limit in 64-bit Windows

v.popolitov@postgrespro.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