HASH_FIXED_SIZE flag gets lost when attaching to existing hash table
Hi hackers,
Recently, while working with hash tables in dynahash.c, I noticed
something weird.
When a hash table is already created in shared memory, and the another
process
calls hash_create attempting to attach to it, it seems like the
HASH_FIXED_SIZE
flag gets lost.
For example, if you start the server compiled with the EXEC_BACKEND
option, and a
hash table with a fixed size is created at the beginning in shared
memory, any
other process started by the postmaster then tries to initialize its
hash table
pointer by calling hash_create with HASH_ATTACH flag. But the table
structure it
points to has 'isfixed' flag set to false, even though the original
table was
created with a HASH_FIXED_SIZE provided.
This could lead to the situation where, when the table's capacity limit
is reached
(which was specified when the table was created), the process will
silently occupy
more of the shared memory (up to a certain point?). Then, another insert
could
trigger an out of shared memory error.
Any thoughts?
Regards,
Aidar Imamov
Attachments:
isfixed_check.patchtext/x-diff; name=isfixed_check.patchDownload
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 1ad155d446e..f0efb4fd3b6 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -496,6 +496,9 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
hashp->ssize = hctl->ssize;
hashp->sshift = hctl->sshift;
+ if (flags & HASH_FIXED_SIZE)
+ hashp->isfixed = true;
+
return hashp;
}
}
Aidar Imamov <a.imamov@postgrespro.ru> writes:
Recently, while working with hash tables in dynahash.c, I noticed
something weird.
When a hash table is already created in shared memory, and the another
process
calls hash_create attempting to attach to it, it seems like the
HASH_FIXED_SIZE
flag gets lost.
Yeah, you are right. This seems to be an oversight in 7c797e719
which introduced that flag. It only affects predicate-lock tables
because we don't use HASH_FIXED_SIZE anywhere else, and it'd only
matter in EXEC_BACKEND builds, so it's not that surprising that
nobody noticed. But we ought to fix it going forward.
I don't really like your solution though. ISTM the intent of the
code is that if the shared hashtable already exists, we adhere to the
properties it has, we don't rely on the current caller to specify the
exact same values. So relying on the caller to get HASH_FIXED_SIZE
correct here seems like the wrong thing. I think we ought to add
an isfixed flag to the shared hashtable header and copy that.
(IOW, isfixed ought to act more like keysize/ssize/sshift, and should
perhaps be grouped with them.)
regards, tom lane
On 2025-07-24 20:24, Tom Lane wrote:
Aidar Imamov <a.imamov@postgrespro.ru> writes:
Recently, while working with hash tables in dynahash.c, I noticed
something weird.
When a hash table is already created in shared memory, and the another
process
calls hash_create attempting to attach to it, it seems like the
HASH_FIXED_SIZE
flag gets lost.Yeah, you are right. This seems to be an oversight in 7c797e719
which introduced that flag. It only affects predicate-lock tables
because we don't use HASH_FIXED_SIZE anywhere else, and it'd only
matter in EXEC_BACKEND builds, so it's not that surprising that
nobody noticed. But we ought to fix it going forward.I don't really like your solution though. ISTM the intent of the
code is that if the shared hashtable already exists, we adhere to the
properties it has, we don't rely on the current caller to specify the
exact same values. So relying on the caller to get HASH_FIXED_SIZE
correct here seems like the wrong thing. I think we ought to add
an isfixed flag to the shared hashtable header and copy that.
(IOW, isfixed ought to act more like keysize/ssize/sshift, and should
perhaps be grouped with them.)regards, tom lane
Thank's, I agree with you.
Attaching an edited version of the patch.
regards,
Aidar Imamov
Attachments:
dynahash_isfixed.patchtext/x-diff; name=dynahash_isfixed.patchDownload
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 1ad155d446e..50069d20fb5 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -195,6 +195,8 @@ struct HASHHDR
long ssize; /* segment size --- must be power of 2 */
int sshift; /* segment shift = log2(ssize) */
int nelem_alloc; /* number of entries to allocate at once */
+ bool isfixed; /* if true, don't enlarge */
+
#ifdef HASH_STATISTICS
@@ -496,6 +498,12 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
hashp->ssize = hctl->ssize;
hashp->sshift = hctl->sshift;
+ /*
+ * make a local copy of the value to determine if the initial
+ * table's size was defined as a hard limit
+ */
+ hashp->isfixed = hctl->isfixed;
+
return hashp;
}
}
@@ -517,6 +525,7 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
errmsg("out of memory")));
}
+ hashp->isfixed = false;
hashp->frozen = false;
hdefault(hashp);
@@ -619,7 +628,8 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
}
if (flags & HASH_FIXED_SIZE)
- hashp->isfixed = true;
+ hashp->isfixed = hctl->isfixed = true;
+
return hashp;
}
@@ -644,6 +654,8 @@ hdefault(HTAB *hashp)
hctl->ssize = DEF_SEGSIZE;
hctl->sshift = DEF_SEGSIZE_SHIFT;
+ hctl->isfixed = false; /* can be enlarged */
+
#ifdef HASH_STATISTICS
hctl->accesses = hctl->collisions = 0;
#endif
Aidar Imamov <a.imamov@postgrespro.ru> writes:
Thank's, I agree with you.
Attaching an edited version of the patch.
Re-reading this in the light of morning, I realized that it can be
done even more simply: let's just move isfixed to the shared struct,
rather than keep two copies. The argument for two copies of keysize
et al is that they're used in hot code paths. But element_alloc()
had better not be a hot code path for a shared hash table, so I
don't think that argument has force for isfixed.
Pushed with that adjustment. I didn't back-patch, because we've
not heard complaints about people running out of shmem on Windows.
If there is anybody running a workload where this would matter,
they might be less happy not more happy about the behavior
changing in a minor release.
regards, tom lane