Re: Patch: New GUC prepared_statement_limit to limit memory used by prepared statements

Am 19.08.2019 um 05:57 schrieb Kyotaro Horiguchi:
> At Sun, 18 Aug 2019 09:43:09 +0200, Daniel Migowski <dmigowski(at)ikoffice(dot)de> wrote in <6e25ca12-9484-8994-a1ee-40fdbe6afa8b(at)ikoffice(dot)de>
>> Am 17.08.2019 um 19:10 schrieb Ibrar Ahmed:
>>> On Sat, Aug 17, 2019 at 6:58 PM Daniel Migowski <dmigowski(at)ikoffice(dot)de
>>> <mailto:dmigowski(at)ikoffice(dot)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.