Date: Tue, 24 Sep 2019 14:05:51 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Melanie Plageman <melanieplageman@gmail.com>,
	Jeff Davis <pgsql@j-davis.com>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Memory Accounting
Message-ID: <20190924115819.5u5i67add5evtea6@development>
References: <027a129b8525601c6a680d27ce3a7172dab61aab.camel@j-davis.com>
 <CAAKRu_aAtcpqFz0fJKAjrvgFUUmYp2mPbeXOPRKWNtJK_Fxa-g@mail.gmail.com>
 <20190724215228.tlyj74ie4l2jn7ji@development>
 <20190924052140.GA1982@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20190924052140.GA1982@paquier.xyz>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Tue, Sep 24, 2019 at 02:21:40PM +0900, Michael Paquier wrote:
>On Wed, Jul 24, 2019 at 11:52:28PM +0200, Tomas Vondra wrote:
>> I think Heikki was asking about places with a lot of sub-contexts, which a
>> completely different issue. It used to be the case that some aggregates
>> created a separate context for each group - like array_agg. That would
>> make Jeff's approach to accounting rather inefficient, because checking
>> how much memory is used would be very expensive (having to loop over a
>> large number of contexts).
>
>The patch has been marked as ready for committer for a week or so, but
>it seems to me that this comment has not been addressed, no?  Are we
>sure that we want this method if it proves to be inefficient when
>there are many sub-contexts and shouldn't we at least test such
>scenarios with a worst-case, customly-made, function?

I don't think so.

Aggregates creating many memory contexts (context for each group) was
discussed extensively in the thread about v11 [1] in 2015. And back then
the conclusion was that that's a pretty awful pattern anyway, as it uses
much more memory (no cross-context freelists), and has various other
issues. In a way, those aggregates are wrong and should be fixed just
like we fixed array_agg/string_agg (even without the memory accounting).

The way I see it we can do either eager or lazy accounting. Eager might
work better for aggregates with many contexts, but it does increase the
overhead for the "regular" aggregates with just one or two contexts.
Considering how rare those many-context aggregates are (I'm not aware of
any such aggregate at the moment), it seems reasonable to pick the lazy
accounting.

(Note: Another factor affecting the lazy vs. eager efficiency is the
number of palloc/pfree calls vs. calls to determine amount of mem used,
but that's mostly orthogonal and we cn ignore it here).

So I think the approach Jeff ended up with sensible - certainly as a
first step. We may improve it in the future, of course, once we have
more practical experience.

Barring objections, I do plan to get this committed by the end of this
CF (i.e. sometime later this week).

[1] https://www.postgresql.org/message-id/1434311039.4369.39.camel%40jeff-desktop

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services