simplehash: preserve consistency in case of OOM
Right now, if allocation fails while growing a hashtable, it's left in
an inconsistent state and can't be used again.
Patch attached.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
Attachments:
v1-0001-simplehash-preserve-consistency-in-case-of-OOM.patchtext/x-patch; charset=UTF-8; name=v1-0001-simplehash-preserve-consistency-in-case-of-OOM.patchDownload
From 82068d744f668039de7249854bc42eead4e77ebc Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Fri, 17 Nov 2023 10:24:03 -0800
Subject: [PATCH v1] simplehash: preserve consistency in case of OOM.
Compute size first, then allocate, then update the structure.
Previously, an out-of-memory when growing could leave the hashtable in
an inconsistent state.
---
src/include/lib/simplehash.h | 39 ++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
index b7adc16b80..517c81bd07 100644
--- a/src/include/lib/simplehash.h
+++ b/src/include/lib/simplehash.h
@@ -128,7 +128,8 @@
#define SH_STAT SH_MAKE_NAME(stat)
/* internal helper functions (no externally visible prototypes) */
-#define SH_COMPUTE_PARAMETERS SH_MAKE_NAME(compute_parameters)
+#define SH_COMPUTE_SIZE SH_MAKE_NAME(compute_size)
+#define SH_UPDATE_PARAMETERS SH_MAKE_NAME(update_parameters)
#define SH_NEXT SH_MAKE_NAME(next)
#define SH_PREV SH_MAKE_NAME(prev)
#define SH_DISTANCE_FROM_OPTIMAL SH_MAKE_NAME(distance)
@@ -303,11 +304,11 @@ SH_SCOPE void SH_STAT(SH_TYPE * tb);
#endif
/*
- * Compute sizing parameters for hashtable. Called when creating and growing
- * the hashtable.
+ * Compute allocation size for hashtable. Result can be passed to
+ * SH_UPDATE_PARAMETERS.
*/
-static inline void
-SH_COMPUTE_PARAMETERS(SH_TYPE * tb, uint64 newsize)
+static inline uint64
+SH_COMPUTE_SIZE(uint64 newsize)
{
uint64 size;
@@ -325,6 +326,18 @@ SH_COMPUTE_PARAMETERS(SH_TYPE * tb, uint64 newsize)
if (unlikely((((uint64) sizeof(SH_ELEMENT_TYPE)) * size) >= SIZE_MAX / 2))
sh_error("hash table too large");
+ return size;
+}
+
+/*
+ * Update sizing parameters for hashtable. Called when creating and growing
+ * the hashtable.
+ */
+static inline void
+SH_UPDATE_PARAMETERS(SH_TYPE * tb, uint64 newsize)
+{
+ uint64 size = SH_COMPUTE_SIZE(newsize);
+
/* now set size */
tb->size = size;
tb->sizemask = (uint32) (size - 1);
@@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data)
/* increase nelements by fillfactor, want to store nelements elements */
size = Min((double) SH_MAX_SIZE, ((double) nelements) / SH_FILLFACTOR);
- SH_COMPUTE_PARAMETERS(tb, size);
+ size = SH_COMPUTE_SIZE(size);
- tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size);
+ tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * size);
+ SH_UPDATE_PARAMETERS(tb, size);
return tb;
}
@@ -490,10 +504,12 @@ SH_GROW(SH_TYPE * tb, uint64 newsize)
Assert(oldsize != SH_MAX_SIZE);
Assert(oldsize < newsize);
- /* compute parameters for new table */
- SH_COMPUTE_PARAMETERS(tb, newsize);
+ newsize = SH_COMPUTE_SIZE(newsize);
+
+ tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * newsize);
- tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size);
+ /* compute parameters for new table */
+ SH_UPDATE_PARAMETERS(tb, newsize);
newdata = tb->data;
@@ -1173,7 +1189,8 @@ SH_STAT(SH_TYPE * tb)
#undef SH_STAT
/* internal function names */
-#undef SH_COMPUTE_PARAMETERS
+#undef SH_COMPUTE_SIZE
+#undef SH_UPDATE_PARAMETERS
#undef SH_COMPARE_KEYS
#undef SH_INITIAL_BUCKET
#undef SH_NEXT
--
2.34.1
Hi,
On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
Right now, if allocation fails while growing a hashtable, it's left in
an inconsistent state and can't be used again.
I'm not against allowing this - but I am curious, in which use cases is this
useful?
@@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data)
/* increase nelements by fillfactor, want to store nelements elements */
size = Min((double) SH_MAX_SIZE, ((double) nelements) / SH_FILLFACTOR);- SH_COMPUTE_PARAMETERS(tb, size); + size = SH_COMPUTE_SIZE(size);- tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); + tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * size);+ SH_UPDATE_PARAMETERS(tb, size);
return tb;
}
Maybe add a comment explaining why it's important to update parameters after
allocating?
Greetings,
Andres Freund
On Fri, 2023-11-17 at 12:13 -0800, Andres Freund wrote:
On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
Right now, if allocation fails while growing a hashtable, it's left
in
an inconsistent state and can't be used again.I'm not against allowing this - but I am curious, in which use cases
is this
useful?
I committed a cache for search_path (f26c2368dc), and afterwards got
concerned that I missed some potential OOM hazards. I separately posted
a patch to fix those (mostly by simplifying things, which in hindsight
was how it should have been done to begin with). Along the way, I also
noticed that simplehash itself is not safe in that case.
I don't think there are other bugs in the system due to simplehash and
OOM, because it's mainly used in the executor.
Please tell me if you think the use of simplehash for a search_path
cache is the wrong tool for the job.
Maybe add a comment explaining why it's important to update
parameters after
allocating?
Will do.
Regards,
Jeff Davis
On Fri, Nov 17, 2023 at 12:13 PM Andres Freund <andres@anarazel.de> wrote:
On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
Right now, if allocation fails while growing a hashtable, it's left in
an inconsistent state and can't be used again.
+1 to the patch.
I'm not against allowing this - but I am curious, in which use cases is this
useful?
I don't have an answer to that, but a guess would be when the server
is dealing with memory pressure. In my view the patch has merit purely
on the grounds of increasing robustness.
@@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data)
/* increase nelements by fillfactor, want to store nelements elements */
size = Min((double) SH_MAX_SIZE, ((double) nelements) / SH_FILLFACTOR);- SH_COMPUTE_PARAMETERS(tb, size); + size = SH_COMPUTE_SIZE(size);- tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); + tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * size);+ SH_UPDATE_PARAMETERS(tb, size);
return tb;
}Maybe add a comment explaining why it's important to update parameters after
allocating?
Perhaps something like this:
+ /*
+ * Update parameters _after_ allocation succeeds; prevent
+ * bogus/corrupted state.
+ */
+ SH_UPDATE_PARAMETERS(tb, size);Best regards,
Gurjeet
http://Gurje.et
On 2023-11-17 13:00:19 -0800, Jeff Davis wrote:
Please tell me if you think the use of simplehash for a search_path
cache is the wrong tool for the job.
No, seems fine. I just was curious - as you said, the older existing users
won't ever care about this case.