Re: [PATCH] Incremental sort (was: PoC: Partial sort)

On Sat, Mar 14, 2020 at 3:58 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Sat, Mar 14, 2020 at 02:41:09PM -0400, James Coleman wrote:
> >
> >It looks like the issue is actually into the `tuplecontext`, which is
> >currently a child context of `sortcontext`:
> >
> >#3 0x0000558cd153b565 in AllocSetCheck
> >(context=context(at)entry=0x558cd28e0b70) at aset.c:1573
> >1573 Assert(total_allocated == context->mem_allocated);
> >(gdb) p total_allocated
> >$1 = 16384
> >(gdb) p context->mem_allocated
> >$2 = 8192
> >(gdb) p context->name
> >$3 = 0x558cd16c8ccd "Caller tuples"
> >
> >I stuck in several more AllocSetCheck calls in aset.c and got the
> >attached backtrace.
> >
>
> I think the problem is pretty simple - tuplesort_reset does call
> tuplesort_reset, which resets the sortcontext. But that *deletes* the
> tuplecontext, so the state->tuplecontext gets stale. I'd haven't looked
> into the exact details, but it clearly confuses the accouting.
>
> The attached patch fixes the issue for me - I'm not claiming it's the
> right fix, but it's the simplest thing I could think of. Maybe the
> tuplesort_rest should work differently, not sure.
>
> And it seems to resolve the memory leak too - I suspect we've freed the
> context (so it was not part of the tree of contexts) but the struct was
> still valid and we kept allocating memory in it - but it was invisible
> to MemoryContextDump etc.

Thanks for tracking that down!

I wondered at first if we should consider making the tuplecontext a
child context of the main context instead, but the allocations it
contains will always live at most the length of the contents of the
sortcontext, so I think that is probably fine as is.

This issue, and the resulting fix, did however make me think that we
have some duplication here that's likely to lead to bugs now and/or
later. I've reworked things a bit so that both (the added for
incremental sort) tuplesort_reset and (the existing)
tuplesort_begin_common now both call out to tuplesort_begin_batch so
configure initial starting state. This way it should be more obvious
that there are both cases to consider if new initialization code is
being added to tuplesort.c.

Working on this refactor I noticed it seems that we were only
reallocing the memtuples array if it was smaller (not sure this is
even possible?) than the initial size, which means if it had been
resized significantly we were keeping that memory tied up for
subsequent batches even if not needed. I suppose one could argue
that's helpful in the sense we don't need to keep increasing its size
on each batch...but it seems to me to be not very clean, so I've
changed that.

I also noticed that we had the USEMEM macro used in tuplesort_reset
regardless of whether or not the the memtuples array had been
realloced, which seems wrong (I don't think we were resetting the
stats), so I changed that too.

I'm still not sure if we should keep the memtuples array in
maincontext (where it is now) or sortcontext. The only argument I can
see for the former is that it allows us a minor optimization: we we
haven't grown the array, we can reuse it for multiple batches. On the
other hand, always resetting it is conceptually more clear. I'm
curious to hear your thoughts on this.

Over at [1] I'd noticed that the patch series versions since my
incorporating Alvaro's pgindent/general formatting patches had been
failing make check. I noted there that I'd found the problem and was
fixing it, so this version of the patch includes that fix. Still (from
that sub-thread) need to figure out what we may or may not need to
update and test with rescan.

James

[1]: https://www.postgresql.org/message-id/CAAaqYe9BsrW%3DDRBOd9yW0s2djofXTM9mRpO%3DLhHrCu4qdGgrVg%40mail.gmail.com