Re: plan with result cache is very slow when work_mem is not enough

On Sat, 8 May 2021 at 08:18, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> pá 7. 5. 2021 v 21:56 odesílatel David Rowley <dgrowleyml@gmail.com> napsal:
>> With USE_ASSERT_CHECKING builds, I did add some code that verifies the
>> memory tracking is set correctly when evicting from the cache. This
>> code is pretty expensive as it loops over the entire cache to check
>> the memory accounting every time we evict something from the cache.
>> Originally, I had this code only run when some other constant was
>> defined, but I ended up changing it to compile that code in for all
>> assert enabled builds.
>>
>> I considered that it might be too expensive as you can see from the
>> comment in [1].  I just wanted to get a few machines other than my own
>> to verify that the memory accounting code was working as expected.
>> There have been no complaints of any Assert failures yet, so maybe
>> it's safe to consider either removing the code entirely or just having
>> it run when some other more specific to purpose constant is defined.
>> If we did the latter, then I'd have concerns that nothing would ever
>> run the code to check the memory accounting, that's why I ended up
>> changing it to run with USE_ASSERT_CHECKING builds.
>
>
> I understand. I think this is too slow for generic assertions, because the overhead is about 50x.

I didn't expect it would show up quite that much.  If you scaled the
test up a bit more and increased work_mem further, then it would be
even more than 50x.

At one point when I was developing the patch, I had two high water
marks for cache memory. When we reached the upper of the two marks,
I'd reduce the memory down to the lower of two marks.  The lower of
the two marks was set to 98% of the higher mark.  In the end, I got
rid of that as I didn't really see what extra overhead there was from
just running the eviction code every time we require another byte.
However, if we did have that again, then the memory checking could
just be done when we run the eviction code. We'd then need to consume
that 2% more memory before it would run again.

My current thinking is that I don't really want to add that complexity
just for some Assert code. I'd only want to do it if there was another
valid reason to.

Another thought I have is that maybe it would be ok just to move
memory accounting debug code so it only runs once in
ExecEndResultCache.  I struggling to imagine that if the memory
tracking did go out of whack, that the problem would have accidentally
fixed itself by the time we got to ExecEndResultCache().  I guess even
if the accounting was counting far too much memory and we'd evicted
everything from the cache to try and get the memory usage down, we'd
still find the problem during ExecEndResultCache(), even if the cache
had become completely empty as a result.

David