Tom Lane писал(а) 2025-01-24 06:13:
v.popoli...@postgrespro.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.
From 015e60140e62439fe6a45afc2c2fcdf464514219 Mon Sep 17 00:00:00 2001
From: Vladlen Popolitov <v.popoli...@postgrespro.ru>
Date: Tue, 28 Jan 2025 18:34:17 +0700
Subject: [PATCH v3] work_mem_vars limit increased in 64bit Windows
---
src/backend/access/gin/ginfast.c | 2 +-
src/backend/access/gin/ginget.c | 2 +-
src/backend/access/hash/hash.c | 4 ++--
src/backend/access/heap/vacuumlazy.c | 2 +-
src/backend/access/nbtree/nbtpage.c | 2 +-
src/backend/commands/vacuumparallel.c | 2 +-
src/backend/executor/execUtils.c | 2 +-
src/backend/executor/nodeBitmapIndexscan.c | 2 +-
src/backend/executor/nodeBitmapOr.c | 2 +-
src/backend/nodes/tidbitmap.c | 8 ++++----
src/backend/optimizer/path/costsize.c | 12 ++++++------
src/backend/replication/logical/reorderbuffer.c | 6 +++---
src/backend/utils/misc/stack_depth.c | 4 ++--
src/backend/utils/sort/tuplestore.c | 2 +-
src/include/nodes/tidbitmap.h | 4 ++--
src/include/utils/guc.h | 3 +--
16 files changed, 29 insertions(+), 30 deletions(-)
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index 97b9684800..73cbbe4bf1 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -456,7 +456,7 @@ ginHeapTupleFastInsert(GinState *ginstate, GinTupleCollector *collector)
* ginInsertCleanup() should not be called inside our CRIT_SECTION.
*/
cleanupSize = GinGetPendingListCleanupSize(index);
- if (metadata->nPendingPages * GIN_PAGE_FREESIZE > cleanupSize * 1024L)
+ if (metadata->nPendingPages * GIN_PAGE_FREESIZE > cleanupSize * (Size)1024)
needCleanup = true;
UnlockReleaseBuffer(metabuffer);
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 330805626e..287f83c763 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -125,7 +125,7 @@ collectMatchBitmap(GinBtreeData *btree, GinBtreeStack *stack,
CompactAttribute *attr;
/* Initialize empty bitmap result */
- scanEntry->matchBitmap = tbm_create(work_mem * 1024L, NULL);
+ scanEntry->matchBitmap = tbm_create(work_mem * (Size)1024, NULL);
/* Null query cannot partial-match anything */
if (scanEntry->isPartialMatch &&
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index f950b9925f..9bde002334 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -120,7 +120,7 @@ hashbuild(Relation heap, Relation index, IndexInfo *indexInfo)
double reltuples;
double allvisfrac;
uint32 num_buckets;
- long sort_threshold;
+ Size sort_threshold;
HashBuildState buildstate;
/*
@@ -155,7 +155,7 @@ hashbuild(Relation heap, Relation index, IndexInfo *indexInfo)
* one page. Also, "initial index size" accounting does not include the
* metapage, nor the first bitmap page.
*/
- sort_threshold = (maintenance_work_mem * 1024L) / BLCKSZ;
+ sort_threshold = (maintenance_work_mem * (Size)1024) / BLCKSZ;
if (index->rd_rel->relpersistence != RELPERSISTENCE_TEMP)
sort_threshold = Min(sort_threshold, NBuffers);
else
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 5ed43e4391..09f8f602d7 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -3037,7 +3037,7 @@ dead_items_alloc(LVRelState *vacrel, int nworkers)
*/
dead_items_info = (VacDeadItemsInfo *) palloc(sizeof(VacDeadItemsInfo));
- dead_items_info->max_bytes = vac_work_mem * 1024L;
+ dead_items_info->max_bytes = vac_work_mem * (Size)1024;
dead_items_info->num_items = 0;
vacrel->dead_items_info = dead_items_info;
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 111675d2ef..f648288bfe 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -2969,7 +2969,7 @@ _bt_pendingfsm_init(Relation rel, BTVacState *vstate, bool cleanuponly)
* int overflow here.
*/
vstate->bufsize = 256;
- maxbufsize = (work_mem * 1024L) / sizeof(BTPendingFSM);
+ maxbufsize = (work_mem * (Size)1024) / sizeof(BTPendingFSM);
maxbufsize = Min(maxbufsize, INT_MAX);
maxbufsize = Min(maxbufsize, MaxAllocSize / sizeof(BTPendingFSM));
/* Stay sane with small work_mem */
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 0d92e694d6..a5361d36e7 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -375,7 +375,7 @@ parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes,
(nindexes_mwm > 0) ?
maintenance_work_mem / Min(parallel_workers, nindexes_mwm) :
maintenance_work_mem;
- shared->dead_items_info.max_bytes = vac_work_mem * 1024L;
+ shared->dead_items_info.max_bytes = vac_work_mem * (size_t)1024;
/* Prepare DSA space for dead items */
dead_items = TidStoreCreateShared(shared->dead_items_info.max_bytes,
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 7c539de5cf..1449d4ee2c 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -325,7 +325,7 @@ CreateWorkExprContext(EState *estate)
Size maxBlockSize = ALLOCSET_DEFAULT_MAXSIZE;
/* choose the maxBlockSize to be no larger than 1/16 of work_mem */
- while (16 * maxBlockSize > work_mem * 1024L)
+ while (16 * maxBlockSize > work_mem * (Size)1024)
maxBlockSize >>= 1;
if (maxBlockSize < ALLOCSET_DEFAULT_INITSIZE)
diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c
index d3ef5a0004..11d8041459 100644
--- a/src/backend/executor/nodeBitmapIndexscan.c
+++ b/src/backend/executor/nodeBitmapIndexscan.c
@@ -91,7 +91,7 @@ MultiExecBitmapIndexScan(BitmapIndexScanState *node)
else
{
/* XXX should we use less than work_mem for this? */
- tbm = tbm_create(work_mem * 1024L,
+ tbm = tbm_create(work_mem * (Size)1024,
((BitmapIndexScan *) node->ss.ps.plan)->isshared ?
node->ss.ps.state->es_query_dsa : NULL);
}
diff --git a/src/backend/executor/nodeBitmapOr.c b/src/backend/executor/nodeBitmapOr.c
index a9ede1c108..aa1b843d1e 100644
--- a/src/backend/executor/nodeBitmapOr.c
+++ b/src/backend/executor/nodeBitmapOr.c
@@ -143,7 +143,7 @@ MultiExecBitmapOr(BitmapOrState *node)
if (result == NULL) /* first subplan */
{
/* XXX should we use less than work_mem for this? */
- result = tbm_create(work_mem * 1024L,
+ result = tbm_create(work_mem * (Size)1024,
((BitmapOr *) node->ps.plan)->isshared ?
node->ps.state->es_query_dsa : NULL);
}
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index af6ac64b3f..cf9952ca1a 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -263,7 +263,7 @@ static int tbm_shared_comparator(const void *left, const void *right,
* be allocated from the DSA.
*/
TIDBitmap *
-tbm_create(long maxbytes, dsa_area *dsa)
+tbm_create(Size maxbytes, dsa_area *dsa)
{
TIDBitmap *tbm;
@@ -1539,10 +1539,10 @@ pagetable_free(pagetable_hash *pagetable, void *pointer)
*
* Estimate number of hashtable entries we can have within maxbytes.
*/
-long
-tbm_calculate_entries(double maxbytes)
+Size
+tbm_calculate_entries(Size maxbytes)
{
- long nbuckets;
+ Size nbuckets;
/*
* Estimate number of hashtable entries we can have within maxbytes. This
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index ec004ed949..632e03966b 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1903,7 +1903,7 @@ cost_tuplesort(Cost *startup_cost, Cost *run_cost,
double input_bytes = relation_byte_size(tuples, width);
double output_bytes;
double output_tuples;
- long sort_mem_bytes = sort_mem * 1024L;
+ int64 sort_mem_bytes = sort_mem * (Size)1024;
/*
* We want to be sure the cost of a sort is never estimated as zero, even
@@ -2488,7 +2488,7 @@ cost_material(Path *path,
Cost startup_cost = input_startup_cost;
Cost run_cost = input_total_cost - input_startup_cost;
double nbytes = relation_byte_size(tuples, width);
- long work_mem_bytes = work_mem * 1024L;
+ double work_mem_bytes = work_mem * (Size)1024;
path->rows = tuples;
@@ -4028,7 +4028,7 @@ final_cost_mergejoin(PlannerInfo *root, MergePath *path,
else if (enable_material && innersortkeys != NIL &&
relation_byte_size(inner_path_rows,
inner_path->pathtarget->width) >
- (work_mem * 1024L))
+ (double)(work_mem * (Size)1024))
path->materialize_inner = true;
else
path->materialize_inner = false;
@@ -4663,7 +4663,7 @@ cost_rescan(PlannerInfo *root, Path *path,
Cost run_cost = cpu_tuple_cost * path->rows;
double nbytes = relation_byte_size(path->rows,
path->pathtarget->width);
- long work_mem_bytes = work_mem * 1024L;
+ double work_mem_bytes = work_mem * (Size)1024;
if (nbytes > work_mem_bytes)
{
@@ -4690,7 +4690,7 @@ cost_rescan(PlannerInfo *root, Path *path,
Cost run_cost = cpu_operator_cost * path->rows;
double nbytes = relation_byte_size(path->rows,
path->pathtarget->width);
- long work_mem_bytes = work_mem * 1024L;
+ double work_mem_bytes = work_mem * (Size)1024;
if (nbytes > work_mem_bytes)
{
@@ -6527,7 +6527,7 @@ compute_bitmap_pages(PlannerInfo *root, RelOptInfo *baserel,
* the bitmap at one time.)
*/
heap_pages = Min(pages_fetched, baserel->pages);
- maxentries = tbm_calculate_entries(work_mem * 1024L);
+ maxentries = (long)tbm_calculate_entries(work_mem * (Size)1024);
if (loop_count > 1)
{
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 79b60df7cf..d35fe8a0f4 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3643,7 +3643,7 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
* haven't exceeded the memory limit.
*/
if (debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_BUFFERED &&
- rb->size < logical_decoding_work_mem * 1024L)
+ rb->size < logical_decoding_work_mem * (Size)1024)
return;
/*
@@ -3656,7 +3656,7 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
* because a user can reduce the logical_decoding_work_mem to a smaller
* value before the most recent change.
*/
- while (rb->size >= logical_decoding_work_mem * 1024L ||
+ while (rb->size >= logical_decoding_work_mem * (Size)1024 ||
(debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE &&
rb->size > 0))
{
@@ -3699,7 +3699,7 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
}
/* We must be under the memory limit now. */
- Assert(rb->size < logical_decoding_work_mem * 1024L);
+ Assert(rb->size < logical_decoding_work_mem * (Size)1024);
}
diff --git a/src/backend/utils/misc/stack_depth.c b/src/backend/utils/misc/stack_depth.c
index 1c79b919f4..acdf0a155e 100644
--- a/src/backend/utils/misc/stack_depth.c
+++ b/src/backend/utils/misc/stack_depth.c
@@ -26,7 +26,7 @@
int max_stack_depth = 100;
/* max_stack_depth converted to bytes for speed of checking */
-static long max_stack_depth_bytes = 100 * 1024L;
+static Size max_stack_depth_bytes = 100 * (Size)1024;
/*
* Stack base pointer -- initialized by set_stack_base(), which
@@ -158,7 +158,7 @@ check_max_stack_depth(int *newval, void **extra, GucSource source)
void
assign_max_stack_depth(int newval, void *extra)
{
- long newval_bytes = newval * 1024L;
+ Size newval_bytes = newval * (Size)1024;
max_stack_depth_bytes = newval_bytes;
}
diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
index aacec8b799..dcd6424db6 100644
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -265,7 +265,7 @@ tuplestore_begin_common(int eflags, bool interXact, int maxKBytes)
state->truncated = false;
state->usedDisk = false;
state->maxSpace = 0;
- state->allowedMem = maxKBytes * 1024L;
+ state->allowedMem = maxKBytes * (Size)1024;
state->availMem = state->allowedMem;
state->myfile = NULL;
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index fedbf69eb5..fb86678844 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -62,7 +62,7 @@ typedef struct TBMIterateResult
/* function prototypes in nodes/tidbitmap.c */
-extern TIDBitmap *tbm_create(long maxbytes, dsa_area *dsa);
+extern TIDBitmap *tbm_create(Size maxbytes, dsa_area *dsa);
extern void tbm_free(TIDBitmap *tbm);
extern void tbm_free_shared_area(dsa_area *dsa, dsa_pointer dp);
@@ -84,7 +84,7 @@ extern void tbm_end_private_iterate(TBMPrivateIterator *iterator);
extern void tbm_end_shared_iterate(TBMSharedIterator *iterator);
extern TBMSharedIterator *tbm_attach_shared_iterate(dsa_area *dsa,
dsa_pointer dp);
-extern long tbm_calculate_entries(double maxbytes);
+extern Size tbm_calculate_entries(Size maxbytes);
extern TBMIterator tbm_begin_iterate(TIDBitmap *tbm,
dsa_area *dsa, dsa_pointer dsp);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 532d6642bb..4492874508 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -18,8 +18,7 @@
/* 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)
--
2.39.5 (Apple Git-154)
