Thanks for having a look at this. On Tue, 7 Nov 2023 at 07:55, Matthias van de Meent <boekewurm+postg...@gmail.com> wrote: > I think it would make sense to split the "add a bump allocator" > changes from the "use the bump allocator in tuplesort" patches.
I've done this and will post updated patches after replying to the other comments. > Tangent: Do we have specific notes on worst-case memory usage of > memory contexts with various allocation patterns? This new bump > allocator seems to be quite efficient, but in a worst-case allocation > pattern it can still waste about 1/3 of its allocated memory due to > never using free space on previous blocks after an allocation didn't > fit on that block. > It probably isn't going to be a huge problem in general, but this > seems like something that could be documented as a potential problem > when you're looking for which allocator to use and compare it with > other allocators that handle different allocation sizes more > gracefully. It might be a good idea to document this. The more memory allocator types we add, the harder it is to decide which one to use when writing new code. > > +++ b/src/backend/utils/mmgr/bump.c > > +BumpBlockIsEmpty(BumpBlock *block) > > +{ > > + /* it's empty if the freeptr has not moved */ > > + return (block->freeptr == (char *) block + Bump_BLOCKHDRSZ); > > [...] > > +static inline void > > +BumpBlockMarkEmpty(BumpBlock *block) > > +{ > > +#if defined(USE_VALGRIND) || defined(CLOBBER_FREED_MEMORY) > > + char *datastart = ((char *) block) + Bump_BLOCKHDRSZ; > > These two use different definitions of the start pointer. Is that deliberate? hmm, I'm not sure if I follow what you mean. Are you talking about the "datastart" variable and the assignment of block->freeptr (which you've not quoted?) > > +++ b/src/include/utils/tuplesort.h > > @@ -109,7 +109,8 @@ typedef struct TuplesortInstrumentation > > * a pointer to the tuple proper (might be a MinimalTuple or IndexTuple), > > * which is a separate palloc chunk --- we assume it is just one chunk and > > * can be freed by a simple pfree() (except during merge, when we use a > > - * simple slab allocator). SortTuples also contain the tuple's first key > > + * simple slab allocator and when performing a non-bounded sort where we > > + * use a bump allocator). SortTuples also contain the tuple's first key > > I'd go with something like the following: > > + * ...(except during merge *where* we use a > + * simple slab allocator, and during a non-bounded sort where we > + * use a bump allocator). Adjusted.