Re: Patch: New GUC prepared_statement_limit to limit memory used byprepared statements

Am 19.08.2019 um 05:57 schrieb Kyotaro Horiguchi:
> At Sun, 18 Aug 2019 09:43:09 +0200, Daniel Migowski <dmigowski@ikoffice.de> wrote in
<6e25ca12-9484-8994-a1ee-40fdbe6afa8b@ikoffice.de>
>> Am 17.08.2019 um 19:10 schrieb Ibrar Ahmed:
>>> On Sat, Aug 17, 2019 at 6:58 PM Daniel Migowski <dmigowski@ikoffice.de
>>> <mailto:dmigowski@ikoffice.de>> wrote:
>>>
>>>
>>>      attached you find a patch that adds a new GUC:
>>>
>>>
>>> Quick questions before looking at the patch.
>>>
>>>
>>>      prepared_statement_limit:
>>>
>>>   - Do we have a consensus about the name of GUC? I don't think it is
>>> the right name for that.
> The almost same was proposed [1] as a part of syscache-pruning
> patch [2], but removed to concentrate on defining how to do that
> on the first instance - syscache.  We have some mechanisms that
> have the same characteristics - can be bloat and no means to keep
> it in a certain size. It is better that they are treated the same
> way, or at least on the same principle.
Correct. However, I don't know the backend well enough to see how to 
unify this. Also time based eviction isn't that important for me, and 
I'd rather work with the memory used. I agree that memory leaks are all 
bad, and a time based eviction for some small cache entries might 
suffice, but CachedPlanSources take up up to 45MB EACH here, and looking 
at the memory directly seems desirable in that case.
> [1] https://www.postgresql.org/message-id/20180315.141246.130742928.horiguchi.kyotaro%40lab.ntt.co.jp
> [2] https://commitfest.postgresql.org/23/931/
>
> Pruning plancaches in any means is valuable, but we haven't
> reached a concsensus on how to do that. My old patch does that
> based on the number of entries because precise memory accounting
> of memory contexts is too expensive. I didn't look this patch
> closer but it seems to use MemoryContext->methods->stats to count
> memory usage, which would be too expensive for the purpose. We
> currently use it only for debug output on critical errors like
> OOM.

Yes, this looks like a place to improve. I hadn't looked at the stats 
function before, and wasn't aware it iterates over all chunks of the 
context. We really don't need all the mem stats, just the total usage 
and this calculates solely from the size of the blocks. Maybe we should 
add this counter (totalmemusage) to the MemoryContexts themselves to 
start to be able to make decisions based on the memory usage fast. 
Shouldn't be too slow because blocks seem to be aquired much less often 
than chunks and when they are aquired an additional add to variable in 
memory wouldn't hurt. One the other hand they should be as fast as 
possible given how often they are used in the database, so that might be 
bad.

Also one could precompute the memory usage of a CachedPlanSource when it 
is saved, when the query_list gets calculated (for plans about to be 
saved) and when the generic plan is stored in it. In combination with a 
fast stats function which only calculates the total usage this looks 
good for me. Also one could store the sum in a session global variable 
to make checking for the need of a prune run faster.

While time based eviction also makes sense in a context where the 
database is not alone on a server, contraining the memory used directly 
attacks the problem one tries to solve with time based eviction.