On Thu, Nov 9, 2023 at 10:32 PM Thomas Munro <thomas.mu...@gmail.com> wrote: > Here is a new attempt to fix this mess. Disclaimer: this based > entirely on reading the manual and vicariously hacking a computer I > don't have via CI.
I'd first like to congratulate this thread on reaching its second birthday. The CommitFest entry hasn't quite made it to the two year mark yet - expect that in another month or so - but thread itself is over the line. Regarding 0001, I don't know if we really need SH_RAW_FREE. You can just define your own SH_FREE implementation in userspace. That doesn't work for SH_RAW_ALLOCATOR because there's code in simplehash.h that knows about memory contexts apart from the actual definition of SH_ALLOCATE - e.g. we include a MemoryContext pointer in SH_TYPE, and in the signature of SH_CREATE. But SH_FREE doesn't seem to have any similar issues. Maybe it's still worth doing for convenience -- I haven't thought about that very hard -- but it doesn't seem to be required in the same way that SH_RAW_ALLOCATOR was. I wonder whether we really want 0002. It seems like a pretty significant behavior change -- now everybody using simplehash has to worry about whether failure cases are possible. And maybe there's some performance overhead. And most of the changes are restricted to the SH_RAW_ALLOCATOR case, but the changes to SH_GROW are not. And making this contingent on SH_RAW_ALLOCATOR doesn't seem principled. I kind of wonder whether trying to handle OOM here is the wrong direction to go. What if we just bail out hard if we can't insert into the hash table? I think that we don't expect the hash table to ever be very large (right?) and we don't install these kinds of defenses everywhere that OOM on a small memory allocation is a possibility (or at least I don't think we do). I'm actually sort of unclear about why you're trying to force this to use raw malloc/free instead of palloc/pfree. Do we need to use this on the frontend side? Do we need it on the backend side prior to the memory context infrastructure being up? -- Robert Haas EDB: http://www.enterprisedb.com
