From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Allow table AM's to cache stuff in relcache
Date: Tue, 30 Jul 2019 22:19:34 +0300
Message-ID: <aa7a58c5-d2ea-ffef-dcab-c56e6b223efe@iki.fi> (raw)
In-Reply-To: <CAOBaU_a9OFjS=OcMam+SVb_eyPNaGT2hkw8HEFo0iWK751jQfQ@mail.gmail.com>
References: <dc0a5fe7-9adc-84f7-40e8-a3cde9529a5f@iki.fi>
<28934.1560526836@sss.pgh.pa.us>
<CAOBaU_a9OFjS=OcMam+SVb_eyPNaGT2hkw8HEFo0iWK751jQfQ@mail.gmail.com>
On 12/07/2019 16:07, Julien Rouhaud wrote:
> On Fri, Jun 14, 2019 at 5:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>>> In the patch, I documented that rd_amcache must be allocated in
>>> CacheMemoryContext, or in rd_indexcxt if it's an index. It works, but
>>> it's a bit weird.
>>
>> Given the way the patch is implemented, it doesn't really matter which
>> context it's in, does it? The retail pfree is inessential but also
>> harmless, if rd_amcache is in rd_indexcxt. So we could take out the
>> "must". I think it's slightly preferable to use rd_indexcxt if available,
>> to reduce the amount of loose junk in CacheMemoryContext.
>
> I agree that for indexes the context used won't make much difference.
> But IMHO avoiding some bloat in CacheMemoryContext is a good enough
> reason to document using rd_indexcxt when available.
Right, it doesn't really matter whether an index AM uses
CacheMemoryContext or rd_indexctx, the code works either way. I think
it's better to give clear advice though, one way or another. Otherwise,
different index AM's can end up doing it differently for no particular
reason, which seems confusing.
>>> It would nice to have one memory context in every
>>> relcache entry, to hold all the stuff related to it, including
>>> rd_amcache. In other words, it would be nice if we had "rd_indexcxt" for
>>> tables, too, not just indexes. That would allow tracking memory usage
>>> more accurately, if you're debugging an out of memory situation for example.
>>
>> We had some discussion related to that in the "hyrax
>> vs. RelationBuildPartitionDesc" thread. I'm not quite sure where
>> we'll settle on that, but some redesign seems inevitable.
>
> There wasn't any progress on this since last month, and this patch
> won't make the situation any worse. I'll mark this patch as ready for
> committer, as it may save some time for people working on custom table
> AM.
Pushed, thanks for the review! As Tom noted, some redesign here seems
inevitable, but this patch shouldn't get in the way of that, so no need
to hold this back for the redesign.
- Heikki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, rjuju123@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: Allow table AM's to cache stuff in relcache
In-Reply-To: <aa7a58c5-d2ea-ffef-dcab-c56e6b223efe@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox