On Sat, 18 Mar 2023 at 13:22, David Rowley <dgrowleyml@gmail.com> wrote:
> It seems to be related to a bug in nodeMemoize.c where we're
> evaluating the cache key expressions in the ExecutorState context. We
> should really be in a more temporary context that gets reset early in
> cache_lookup() before the call to prepare_probe_slot(). I'll need to
> look in a bit more detail about what that context actually should be.
I've attached fix_memoize_memory_leak.patch to address this.
Using your test case, here are the memory stats before and after the
fix (taken during ExecEndMemoize).
Before:
TopPortalContext: 8192 total in 1 blocks; 7656 free (0 chunks); 536 used
PortalHoldContext: 24640 total in 2 blocks; 7384 free (0 chunks); 17256 used
PortalContext: 51264 total in 10 blocks; 7320 free (11 chunks);
43944 used: <unnamed>
ExecutorState: 1770040136 total in 223 blocks; 3728312 free (87
chunks); 1766311824 used
MemoizeHashTable: 46137408 total in 15 blocks; 6353568 free (5
chunks); 39783840 used
ExprContext: 8192 total in 1 blocks; 3512 free (0 chunks); 4680 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
After:
TopPortalContext: 8192 total in 1 blocks; 7656 free (0 chunks); 536 used
PortalHoldContext: 24640 total in 2 blocks; 7384 free (0 chunks); 17256 used
PortalContext: 51264 total in 10 blocks; 7320 free (11 chunks);
43944 used: <unnamed>
ExecutorState: 76616 total in 5 blocks; 13528 free (8 chunks); 63088 used
MemoizeHashTable: 46137408 total in 15 blocks; 6353568 free (5
chunks); 39783840 used
ExprContext: 8192 total in 1 blocks; 3512 free (0 chunks); 4680 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
> Another thing that came to mind is that we don't track the memory for
> the cache key. So that could account for some additional memory usage
> with Memoize. I have a patch locally to fix that. Likely that would be
> a master-only fix, however. I doubt that's accounting for much of the
> extra memory you're reporting anyway. In hindsight, we should be
> tracking that, but I think at the time I was writing this code, I had
> thoughts that it wasn't much memory compared to storing the cached
> tuples. I now think differently.
I've also attached the have_memoize_track_cachekey_memory.patch to
address this. I intend this one for master only. I considered if
maybe the executor changes without the planner changes could be
backpatched, but I don't think that's a good idea. It wouldn't cause
plan stability problems, but it could cause executor performance
changes if we start evicting more cache entries due to memory
pressure.
David