pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
Allow table AM's to cache stuff in relcache 4+ messages / 3 participants [nested] [flat]
* Allow table AM's to cache stuff in relcache @ 2019-06-14 15:20 Heikki Linnakangas <hlinnaka@iki.fi> 2019-06-14 15:40 ` Re: Allow table AM's to cache stuff in relcache Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 1 reply; 4+ messages in thread From: Heikki Linnakangas @ 2019-06-14 15:20 UTC (permalink / raw) To: pgsql-hackers Index AM's can cache stuff in RelationData->rd_amcache. In the zedstore table AM we've been hacking on, I'd like to also use rd_amcache to cache some information, but that's not currently possible, because rd_amcache can only be used by index AMs, not table AMs. Attached patch allows rd_amcache to also be used by table AMs. While working on this, I noticed that the memory management of relcache entries is quite complicated. Most stuff that's part of a relcache entry is allocated in CacheMemoryContext. But some fields have a dedicated memory context to hold them, like rd_rulescxt for rules and rd_pdcxt for partition information. And indexes have rd_indexcxt to hold all kinds of index support info. 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. 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. However, the special contexts like rd_rulescxt and rd_pdcxt would still be needed, because of the way RelationClearRelation preserves them, when rebuilding the relcache entry for an open relation. So I'm not sure how much it would really simplify things. Also, there's some overhead for having extra memory contexts, and some people already complain that the relcache uses too much memory. Alternatively, we could document that rd_amcache should always be allocated in CacheMemoryContext, even for indexes. That would make the rule for pg_amcache straightforward. There's no particular reason why rd_amcache has to be allocated in rd_indexcxt, except for how it's accounted for in memory context dumps. - Heikki Attachments: [text/x-patch] 0001-Allow-table-AM-s-to-use-rd_amcache-too.patch (3.7K, ../../dc0a5fe7-9adc-84f7-40e8-a3cde9529a5f@iki.fi/2-0001-Allow-table-AM-s-to-use-rd_amcache-too.patch) download | inline diff: From 0b061f3c0ec8f8dedaf30b4e3cefc0cb24630ced Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: Fri, 14 Jun 2019 18:09:19 +0300 Subject: [PATCH 1/1] Allow table AM's to use rd_amcache, too. The rd_amcache allows an index AM to cache arbitrary information in a relcache entry. This commit moves the cleanup of rd_amcache so that it's also available for table AMs. --- src/backend/utils/cache/relcache.c | 11 +++++++++-- src/include/utils/rel.h | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 2b992d78327..204e1eb7aad 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -2358,6 +2358,10 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc) pfree(relation->rd_options); if (relation->rd_indextuple) pfree(relation->rd_indextuple); + if (relation->rd_amcache) + pfree(relation->rd_amcache); + if (relation->rd_fdwroutine) + pfree(relation->rd_fdwroutine); if (relation->rd_indexcxt) MemoryContextDelete(relation->rd_indexcxt); if (relation->rd_rulescxt) @@ -2370,8 +2374,6 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc) MemoryContextDelete(relation->rd_pdcxt); if (relation->rd_partcheckcxt) MemoryContextDelete(relation->rd_partcheckcxt); - if (relation->rd_fdwroutine) - pfree(relation->rd_fdwroutine); pfree(relation); } @@ -2416,6 +2418,11 @@ RelationClearRelation(Relation relation, bool rebuild) */ RelationCloseSmgr(relation); + /* Free AM cached data, if any */ + if (relation->rd_amcache) + pfree(relation->rd_amcache); + relation->rd_amcache = NULL; + /* * Treat nailed-in system relations separately, they always need to be * accessible, so we can't blow them away. diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index d7f33abce3f..9080a3c1609 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -152,13 +152,6 @@ typedef struct RelationData * those with lefttype and righttype equal to the opclass's opcintype. The * arrays are indexed by support function number, which is a sufficient * identifier given that restriction. - * - * Note: rd_amcache is available for index AMs to cache private data about - * an index. This must be just a cache since it may get reset at any time - * (in particular, it will get reset by a relcache inval message for the - * index). If used, it must point to a single memory chunk palloc'd in - * rd_indexcxt. A relcache reset will include freeing that chunk and - * setting rd_amcache = NULL. */ MemoryContext rd_indexcxt; /* private memory cxt for this stuff */ /* use "struct" here to avoid needing to include amapi.h: */ @@ -173,9 +166,19 @@ typedef struct RelationData Oid *rd_exclops; /* OIDs of exclusion operators, if any */ Oid *rd_exclprocs; /* OIDs of exclusion ops' procs, if any */ uint16 *rd_exclstrats; /* exclusion ops' strategy numbers, if any */ - void *rd_amcache; /* available for use by index AM */ Oid *rd_indcollation; /* OIDs of index collations */ + /* + * rd_amcache is available for index and table AMs to cache private data + * about the relation. This must be just a cache since it may get reset + * at any time (in particular, it will get reset by a relcache inval + * message for the relation). If used, it must point to a single memory + * chunk palloc'd in CacheMemoryContext, or in rd_indexcxt for an index + * relation. A relcache reset will include freeing that chunk and setting + * rd_amcache = NULL. + */ + void *rd_amcache; /* available for use by index/table AM */ + /* * foreign-table support * -- 2.20.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Allow table AM's to cache stuff in relcache 2019-06-14 15:20 Allow table AM's to cache stuff in relcache Heikki Linnakangas <hlinnaka@iki.fi> @ 2019-06-14 15:40 ` Tom Lane <tgl@sss.pgh.pa.us> 2019-07-12 13:07 ` Re: Allow table AM's to cache stuff in relcache Julien Rouhaud <rjuju123@gmail.com> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2019-06-14 15:40 UTC (permalink / raw) To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers Heikki Linnakangas <hlinnaka@iki.fi> writes: > Index AM's can cache stuff in RelationData->rd_amcache. In the zedstore > table AM we've been hacking on, I'd like to also use rd_amcache to cache > some information, but that's not currently possible, because rd_amcache > can only be used by index AMs, not table AMs. > Attached patch allows rd_amcache to also be used by table AMs. Seems reasonable. > 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. > 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. regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Allow table AM's to cache stuff in relcache 2019-06-14 15:20 Allow table AM's to cache stuff in relcache Heikki Linnakangas <hlinnaka@iki.fi> 2019-06-14 15:40 ` Re: Allow table AM's to cache stuff in relcache Tom Lane <tgl@sss.pgh.pa.us> @ 2019-07-12 13:07 ` Julien Rouhaud <rjuju123@gmail.com> 2019-07-30 19:19 ` Re: Allow table AM's to cache stuff in relcache Heikki Linnakangas <hlinnaka@iki.fi> 0 siblings, 1 reply; 4+ messages in thread From: Julien Rouhaud @ 2019-07-12 13:07 UTC (permalink / raw) To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers On Fri, Jun 14, 2019 at 5:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > Index AM's can cache stuff in RelationData->rd_amcache. In the zedstore > > table AM we've been hacking on, I'd like to also use rd_amcache to cache > > some information, but that's not currently possible, because rd_amcache > > can only be used by index AMs, not table AMs. > > Attached patch allows rd_amcache to also be used by table AMs. > > Seems reasonable. +1. > > 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. > > 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. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Allow table AM's to cache stuff in relcache 2019-06-14 15:20 Allow table AM's to cache stuff in relcache Heikki Linnakangas <hlinnaka@iki.fi> 2019-06-14 15:40 ` Re: Allow table AM's to cache stuff in relcache Tom Lane <tgl@sss.pgh.pa.us> 2019-07-12 13:07 ` Re: Allow table AM's to cache stuff in relcache Julien Rouhaud <rjuju123@gmail.com> @ 2019-07-30 19:19 ` Heikki Linnakangas <hlinnaka@iki.fi> 0 siblings, 0 replies; 4+ messages in thread From: Heikki Linnakangas @ 2019-07-30 19:19 UTC (permalink / raw) To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers 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 ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2019-07-30 19:19 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-06-14 15:20 Allow table AM's to cache stuff in relcache Heikki Linnakangas <hlinnaka@iki.fi> 2019-06-14 15:40 ` Tom Lane <tgl@sss.pgh.pa.us> 2019-07-12 13:07 ` Julien Rouhaud <rjuju123@gmail.com> 2019-07-30 19:19 ` Heikki Linnakangas <hlinnaka@iki.fi>
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