hyrax vs. RelationBuildPartitionDesc

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

hyrax vs. RelationBuildPartitionDesc

Robert Haas
Hi,

Amit Kapila pointed out to be that there are some buidfarm failures on
hyrax which seem to have started happening around the time I committed
898e5e3290a72d288923260143930fb32036c00c.  It failed like this once:

2019-03-07 19:57:40.231 EST [28073:11] DETAIL:  Failed process was
running: /* same as above */
        explain (costs off) select * from rlp where a = 1::numeric;

And then the next three runs failed like this:

2019-03-09 04:56:04.939 EST [1727:4] LOG:  server process (PID 2898)
was terminated by signal 9: Killed
2019-03-09 04:56:04.939 EST [1727:5] DETAIL:  Failed process was
running: UPDATE range_parted set c = (case when c = 96 then 110 else c
+ 1 end ) WHERE a = 'b' and b > 10 and c >= 96;

I couldn't think of an explanation for this other than that the
process involved had used a lot of memory and gotten killed by the OOM
killer, and it turns out that RelationBuildPartitionDesc leaks memory
into the surrounding context every time it's called.  It's not a lot
of memory, but in a CLOBBER_CACHE_ALWAYS build it adds up, because
this function gets called a lot.  All of this is true even before the
commit in question, but for some reason that I don't understand that
commit makes it worse.

I tried having that function use a temporary context for its scratch
space and that causes a massive drop in memory usage when running
update.sql frmo the regression tests under CLOBBER_CACHE_ALWAYS.  I
ran MacOS X's vmmap tool to see the impact, measuring the size of the
"DefaultMallocZone".  Without this patch, that peaks at >450MB; with
this patch it peaks ~270MB.  There is a significant reduct in typical
memory usage, too.  It's noticeably better with this patch than it was
before 898e5e3290a72d288923260143930fb32036c00c.

I'm not sure whether this is the right way to address the problem.
RelationBuildPartitionDesc() creates basically all of the data
structures it needs and then copies them into rel->rd_pdcxt, which has
always seemed a bit inefficient to me.  Another way to redesign this
would be to have the function create a temporary context, do all of
its work there, and then reparent the context under CacheMemoryContext
at the end.  That means any leaks would go into a relatively
long-lifespan context, but on the other hand you wouldn't leak into
the same context a zillion times over, and you'd save the expense of
copying everything.  I think that the biggest thing that is being
copied around here is the partition bounds, so maybe the leak wouldn't
amount to much, and we could also do things like list_free(inhoids) to
make it a little tighter.

Opinions?  Suggestions?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

rbcontext.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Alvaro Herrera-9
On 2019-Mar-13, Robert Haas wrote:

> RelationBuildPartitionDesc() creates basically all of the data
> structures it needs and then copies them into rel->rd_pdcxt, which has
> always seemed a bit inefficient to me.  Another way to redesign this
> would be to have the function create a temporary context, do all of
> its work there, and then reparent the context under CacheMemoryContext
> at the end.  That means any leaks would go into a relatively
> long-lifespan context, but on the other hand you wouldn't leak into
> the same context a zillion times over, and you'd save the expense of
> copying everything.  I think that the biggest thing that is being
> copied around here is the partition bounds, so maybe the leak wouldn't
> amount to much, and we could also do things like list_free(inhoids) to
> make it a little tighter.

I remember going over this code's memory allocation strategy a bit to
avoid the copy while not incurring potential leaks CacheMemoryContext;
as I recall, my idea was to use two contexts, one of which is temporary
and used for any potentially leaky callees, and destroyed at the end of
the function, and the other contains the good stuff and is reparented to
CacheMemoryContext at the end.  So if you have any accidental leaks,
they don't affect a long-lived context.  You have to be mindful of not
calling leaky code when you're using the permanent one.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
On Wed, Mar 13, 2019 at 12:42 PM Alvaro Herrera
<[hidden email]> wrote:
> I remember going over this code's memory allocation strategy a bit to
> avoid the copy while not incurring potential leaks CacheMemoryContext;
> as I recall, my idea was to use two contexts, one of which is temporary
> and used for any potentially leaky callees, and destroyed at the end of
> the function, and the other contains the good stuff and is reparented to
> CacheMemoryContext at the end.  So if you have any accidental leaks,
> they don't affect a long-lived context.  You have to be mindful of not
> calling leaky code when you're using the permanent one.

Well, that assumes that the functions which allocate the good stuff do
not also leak, which seems a bit fragile.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Tom Lane-2
Robert Haas <[hidden email]> writes:

> On Wed, Mar 13, 2019 at 12:42 PM Alvaro Herrera
> <[hidden email]> wrote:
>> I remember going over this code's memory allocation strategy a bit to
>> avoid the copy while not incurring potential leaks CacheMemoryContext;
>> as I recall, my idea was to use two contexts, one of which is temporary
>> and used for any potentially leaky callees, and destroyed at the end of
>> the function, and the other contains the good stuff and is reparented to
>> CacheMemoryContext at the end.  So if you have any accidental leaks,
>> they don't affect a long-lived context.  You have to be mindful of not
>> calling leaky code when you're using the permanent one.

> Well, that assumes that the functions which allocate the good stuff do
> not also leak, which seems a bit fragile.

I'm a bit confused as to why there's an issue here at all.  The usual
plan for computed-on-demand relcache sub-structures is that we compute
a working copy that we're going to return to the caller using the
caller's context (which is presumably statement-duration at most)
and then do the equivalent of copyObject to stash a long-lived copy
into the relcache context.  Is this case being done differently, and if
so why?  If it's being done the same, where are we leaking?

I recall having noticed someplace where I thought the relcache partition
support was simply failing to make provisions for cleaning up a cached
structure at relcache entry drop, but I didn't have time to pursue it
right then.  Let me see if I can reconstruct what I was worried about.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
On Wed, Mar 13, 2019 at 1:15 PM Tom Lane <[hidden email]> wrote:
> I'm a bit confused as to why there's an issue here at all.  The usual
> plan for computed-on-demand relcache sub-structures is that we compute
> a working copy that we're going to return to the caller using the
> caller's context (which is presumably statement-duration at most)
> and then do the equivalent of copyObject to stash a long-lived copy
> into the relcache context.  Is this case being done differently, and if
> so why?  If it's being done the same, where are we leaking?

It's being done in just that way.  The caller's context is
MessageContext, which is indeed statement duration.  But if you plunk
10k into MessageContext a few thousand times per statement, then you
chew through a couple hundred meg of memory, and apparently hyrax
can't tolerate that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Tom Lane-2
In reply to this post by Tom Lane-2
I wrote:
> I recall having noticed someplace where I thought the relcache partition
> support was simply failing to make provisions for cleaning up a cached
> structure at relcache entry drop, but I didn't have time to pursue it
> right then.  Let me see if I can reconstruct what I was worried about.

Ah, here we are: it was rel->rd_partcheck.  I'm not sure exactly how
complicated that structure can be, but I'm pretty sure that this is
a laughably inadequate method of cleaning it up:

        if (relation->rd_partcheck)
                pfree(relation->rd_partcheck);

Having it be loose data in CacheMemoryContext, which is the current state
of affairs, is just not going to be practical to clean up.  I suggest that
maybe it could be copied into rd_partkeycxt or rd_pdcxt, so that it'd go
away as a byproduct of freeing those.  If there's a reason it has to be
independent of both, it'll have to have its own small context.

Dunno if that's related to hyrax's issue, though.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Alvaro Herrera-9
In reply to this post by Robert Haas
On 2019-Mar-13, Robert Haas wrote:

> On Wed, Mar 13, 2019 at 12:42 PM Alvaro Herrera
> <[hidden email]> wrote:
> > I remember going over this code's memory allocation strategy a bit to
> > avoid the copy while not incurring potential leaks CacheMemoryContext;
> > as I recall, my idea was to use two contexts, one of which is temporary
> > and used for any potentially leaky callees, and destroyed at the end of
> > the function, and the other contains the good stuff and is reparented to
> > CacheMemoryContext at the end.  So if you have any accidental leaks,
> > they don't affect a long-lived context.  You have to be mindful of not
> > calling leaky code when you're using the permanent one.
>
> Well, that assumes that the functions which allocate the good stuff do
> not also leak, which seems a bit fragile.

A bit, yes, but not overly so, and it's less fragile that not having
such a protection.  Anything that allocates in CacheMemoryContext needs
to be very careful anyway.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera <[hidden email]> wrote:
> A bit, yes, but not overly so, and it's less fragile that not having
> such a protection.  Anything that allocates in CacheMemoryContext needs
> to be very careful anyway.

True, but I think it's more fragile than either of the options I proposed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
In reply to this post by Tom Lane-2
On Wed, Mar 13, 2019 at 1:30 PM Tom Lane <[hidden email]> wrote:
> Ah, here we are: it was rel->rd_partcheck.  I'm not sure exactly how
> complicated that structure can be, but I'm pretty sure that this is
> a laughably inadequate method of cleaning it up:
>
>         if (relation->rd_partcheck)
>                 pfree(relation->rd_partcheck);

Oh, for crying out loud.

> Dunno if that's related to hyrax's issue, though.

It's related in the sense that it's a leak, and any leak will tend to
run the system out of memory more easily, but what I observed was a
large leak into MessageContext, and that would be a leak into
CacheMemoryContext, so I think it's probably a sideshow rather than
the main event.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Alvaro Herrera-9
In reply to this post by Robert Haas
On 2019-Mar-13, Robert Haas wrote:

> On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera <[hidden email]> wrote:
> > A bit, yes, but not overly so, and it's less fragile that not having
> > such a protection.  Anything that allocates in CacheMemoryContext needs
> > to be very careful anyway.
>
> True, but I think it's more fragile than either of the options I proposed.

You do?  Unless I misunderstood, your options are:

1. (the patch you attached) create a temporary memory context that is
used for everything, then at the end copy the good stuff to CacheMemCxt
(or a sub-context thereof).  This still needs to copy.

2. create a temp memory context, do everything there, do retail freeing
of everything we don't want, reparenting the context to CacheMemCxt.
Hope we didn't forget to pfree anything.

How is any of those superior to what I propose?

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Tom Lane-2
In reply to this post by Robert Haas
Robert Haas <[hidden email]> writes:
>> Dunno if that's related to hyrax's issue, though.

> It's related in the sense that it's a leak, and any leak will tend to
> run the system out of memory more easily, but what I observed was a
> large leak into MessageContext, and that would be a leak into
> CacheMemoryContext, so I think it's probably a sideshow rather than
> the main event.

OK, in that case it's definitely all the temporary data that gets created
that is the problem.  I've not examined your patch in great detail but
it looks plausible for fixing that.

I think that RelationBuildPartitionDesc could use some additional cleanup
or at least better commenting.  In particular, it's neither documented nor
obvious to the naked eye why rel->rd_partdesc mustn't get set till the
very end.  As the complainant, I'm willing to go fix that, but do you want
to push your patch first so it doesn't get broken?  Or I could include
your patch in the cleanup.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Tom Lane-2
In reply to this post by Alvaro Herrera-9
Alvaro Herrera <[hidden email]> writes:
> You do?  Unless I misunderstood, your options are:

> 1. (the patch you attached) create a temporary memory context that is
> used for everything, then at the end copy the good stuff to CacheMemCxt
> (or a sub-context thereof).  This still needs to copy.

> 2. create a temp memory context, do everything there, do retail freeing
> of everything we don't want, reparenting the context to CacheMemCxt.
> Hope we didn't forget to pfree anything.

> How is any of those superior to what I propose?

I doubt that what you're suggesting is terribly workable.  It's not
just RelationBuildPartitionDesc that's at issue.  Part of what will
be the long-lived data structure is made by partition_bounds_create,
and that invokes quite a boatload of code that both makes the final
data structure and leaks a lot of intermediate junk.  Having to be
very careful about permanent vs temporary data throughout all of that
seems like a recipe for bugs.

The existing code in RelationBuildPartitionDesc is already pretty good
about avoiding copying of data other than the output of
partition_bounds_create.  In fact, I think it's already close to what
you're suggesting other than that point.  So I think --- particularly
given that we need something we can back-patch into v11 --- that we
shouldn't try to do anything much more complicated than what Robert is
suggesting.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
In reply to this post by Alvaro Herrera-9
On Wed, Mar 13, 2019 at 2:21 PM Alvaro Herrera <[hidden email]> wrote:

> On 2019-Mar-13, Robert Haas wrote:
> > On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera <[hidden email]> wrote:
> > > A bit, yes, but not overly so, and it's less fragile that not having
> > > such a protection.  Anything that allocates in CacheMemoryContext needs
> > > to be very careful anyway.
> >
> > True, but I think it's more fragile than either of the options I proposed.
>
> You do?  Unless I misunderstood, your options are:
>
> 1. (the patch you attached) create a temporary memory context that is
> used for everything, then at the end copy the good stuff to CacheMemCxt
> (or a sub-context thereof).  This still needs to copy.
>
> 2. create a temp memory context, do everything there, do retail freeing
> of everything we don't want, reparenting the context to CacheMemCxt.
> Hope we didn't forget to pfree anything.
>
> How is any of those superior to what I propose?

Well, *I* thought of it, so obviously it must be superior.  :-)

More seriously, your idea does seem better in some ways.  My #1
doesn't avoid the copy, but does kill the leaks.  My #2 avoids the
copy, but risks a different flavor of leakage.  Your idea also avoids
the copy and leaks in fewer cases than my #2.  In that sense, it is
the technically superior option.  However, it also requires more
memory context switches than either of my options, and I guess that
seems fragile to me in the sense that I think future code changes are
more likely to go wrong due to the complexity involved.  I might be
mistaken about that, though.

One other note is that, although the extra copy looks irksome, it's
probably not very significant from a performance point of view.  In a
non-CLOBBER_CACHE_ALWAYS build it doesn't happen frequently enough to
matter, and in a CLOBBER_CACHE_ALWAYS build everything is already so
slow that it still doesn't matter.  That's not the only consideration,
though.  Code which copies data structures might be buggy, or might
develop bugs in the future, and avoiding that copy would avoid
exposure to such bugs.  On the other hand, it's not really possible to
remove the copying without increasing the risk of leaking into the
long-lived context.

In some ways, I think this is all a natural outgrowth of the fact that
we rely on palloc() in so many places instead of forcing code to be
explicit about which memory context it intends to target.  Global
variables are considered harmful, and it's not that difficult to see
the connection between that general principle and present
difficulties.  However, I will not have time to write and debug a
patch reversing that choice between now and feature freeze.  :-)

I'm kinda inclined to go for the brute-force approach of slamming the
temporary context in there as in the patch I proposed upthread, which
should solve the immediate problem, and we can implement one of these
other ideas later if we want.  What do you think about that?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
In reply to this post by Tom Lane-2
On Wed, Mar 13, 2019 at 2:26 PM Tom Lane <[hidden email]> wrote:
> OK, in that case it's definitely all the temporary data that gets created
> that is the problem.  I've not examined your patch in great detail but
> it looks plausible for fixing that.

Cool.

> I think that RelationBuildPartitionDesc could use some additional cleanup
> or at least better commenting.  In particular, it's neither documented nor
> obvious to the naked eye why rel->rd_partdesc mustn't get set till the
> very end.  As the complainant, I'm willing to go fix that, but do you want
> to push your patch first so it doesn't get broken?  Or I could include
> your patch in the cleanup.

Yeah, that probably makes sense, though it might be polite to wait
another hour or two to see if anyone wants to argue with that approach
further.

It seems kinda obvious to me why rel->rd_partdesc can't get set until
the end.  Isn't it just that you'd better not set a permanent pointer
to a data structure until you're past any code that might ERROR, which
is pretty much everything?  That principle applies to lots of
PostgreSQL code, not just this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Wed, Mar 13, 2019 at 2:26 PM Tom Lane <[hidden email]> wrote:
>> I think that RelationBuildPartitionDesc could use some additional cleanup
>> or at least better commenting.  In particular, it's neither documented nor
>> obvious to the naked eye why rel->rd_partdesc mustn't get set till the
>> very end.  As the complainant, I'm willing to go fix that, but do you want
>> to push your patch first so it doesn't get broken?  Or I could include
>> your patch in the cleanup.

> It seems kinda obvious to me why rel->rd_partdesc can't get set until
> the end.  Isn't it just that you'd better not set a permanent pointer
> to a data structure until you're past any code that might ERROR, which
> is pretty much everything?  That principle applies to lots of
> PostgreSQL code, not just this.

Yeah, but usually there's some comment pointing it out.  I also wonder
if there aren't corner-case bugs; it seems a bit bogus for example that
rd_pdcxt is created without any thought as to whether it might be set
already.  It's not clear whether this has been written with the
level of paranoia that's appropriate for messing with a relcache entry,
and some comments would make it a lot clearer (a) if that is true and
(b) what assumptions are implicitly being shared with relcache.c.

Meanwhile, who's going to take point on cleaning up rd_partcheck?
I don't really understand this code well enough to know whether that
can share one of the existing partitioning-related sub-contexts.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
On Wed, Mar 13, 2019 at 3:14 PM Tom Lane <[hidden email]> wrote:
> Yeah, but usually there's some comment pointing it out.  I also wonder
> if there aren't corner-case bugs; it seems a bit bogus for example that
> rd_pdcxt is created without any thought as to whether it might be set
> already.  It's not clear whether this has been written with the
> level of paranoia that's appropriate for messing with a relcache entry,
> and some comments would make it a lot clearer (a) if that is true and
> (b) what assumptions are implicitly being shared with relcache.c.

Yeah, this is all pretty new code, and it probably has some bugs we
haven't found yet.

> Meanwhile, who's going to take point on cleaning up rd_partcheck?
> I don't really understand this code well enough to know whether that
> can share one of the existing partitioning-related sub-contexts.

Well, it would be nice if Amit Langote worked on it since he wrote the
original version of most of this code, but I'm sure he'll defer to you
if you feel the urge to work on it, or I can take a look at it (not
today).

To your question, I think it probably can't share a context.  Briefly,
rd_partkey can't change ever, except that when a partitioned relation
is in the process of being created it is briefly NULL; once it obtains
a value, that value cannot be changed.  If you want to range-partition
a list-partitioned table or something like that, you have to drop the
table and create a new one.  I think that's a perfectly acceptable
permanent limitation and I have no urge whatever to change it.
rd_partdesc changes when you attach or detach a child partition.
rd_partcheck is the reverse: it changes when you attach or detach this
partition to or from a parent.  It's probably helpful to think of the
case of a table with partitions each of which is itself partitioned --
the table at that middle level has to worry both about gaining or
losing children and about being ripped away from its parent.

As a parenthetical note, I observe that relcache.c seems to know
almost nothing about rd_partcheck.  rd_partkey and rd_partdesc both
have handling in RelationClearRelation(), but rd_partcheck does not,
and I suspect that's wrong.  So the problems are probably not confined
to the relcache-drop-time problem that you observed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Wed, Mar 13, 2019 at 3:14 PM Tom Lane <[hidden email]> wrote:
>> Meanwhile, who's going to take point on cleaning up rd_partcheck?
>> I don't really understand this code well enough to know whether that
>> can share one of the existing partitioning-related sub-contexts.

> To your question, I think it probably can't share a context.  Briefly,
> rd_partkey can't change ever, except that when a partitioned relation
> is in the process of being created it is briefly NULL; once it obtains
> a value, that value cannot be changed.  If you want to range-partition
> a list-partitioned table or something like that, you have to drop the
> table and create a new one.  I think that's a perfectly acceptable
> permanent limitation and I have no urge whatever to change it.
> rd_partdesc changes when you attach or detach a child partition.
> rd_partcheck is the reverse: it changes when you attach or detach this
> partition to or from a parent.

Got it.  Yeah, it seems like the clearest and least bug-prone solution
is for those to be in three separate sub-contexts.  The only reason
I was trying to avoid that was the angle of how to back-patch into 11.
However, we can just add the additional context pointer field at the
end of the Relation struct in v11, and that should be good enough to
avoid ABI problems.

Off topic for the moment, since this clearly wouldn't be back-patch
material, but I'm starting to wonder if we should just have a context
for each relcache entry and get rid of most or all of the retail
cleanup logic in RelationDestroyRelation ...

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
On Wed, Mar 13, 2019 at 4:18 PM Tom Lane <[hidden email]> wrote:
> Off topic for the moment, since this clearly wouldn't be back-patch
> material, but I'm starting to wonder if we should just have a context
> for each relcache entry and get rid of most or all of the retail
> cleanup logic in RelationDestroyRelation ...

I think that idea might have a lot of merit, but I haven't studied it closely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Robert Haas
On Wed, Mar 13, 2019 at 4:38 PM Robert Haas <[hidden email]> wrote:
> On Wed, Mar 13, 2019 at 4:18 PM Tom Lane <[hidden email]> wrote:
> > Off topic for the moment, since this clearly wouldn't be back-patch
> > material, but I'm starting to wonder if we should just have a context
> > for each relcache entry and get rid of most or all of the retail
> > cleanup logic in RelationDestroyRelation ...
>
> I think that idea might have a lot of merit, but I haven't studied it closely.

It just occurred to me that one advantage of this would be that you
could see how much memory was being used by each relcache entry using
MemoryContextStats(), which seems super-appealing.  In fact, what
about getting rid of all allocations in CacheMemoryContext itself in
favor of some more specific context in each case?  That would make it
a lot clearer where to look for leaks -- or efficiencies.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: hyrax vs. RelationBuildPartitionDesc

Andres Freund
Hi,

On 2019-03-13 16:50:53 -0400, Robert Haas wrote:

> On Wed, Mar 13, 2019 at 4:38 PM Robert Haas <[hidden email]> wrote:
> > On Wed, Mar 13, 2019 at 4:18 PM Tom Lane <[hidden email]> wrote:
> > > Off topic for the moment, since this clearly wouldn't be back-patch
> > > material, but I'm starting to wonder if we should just have a context
> > > for each relcache entry and get rid of most or all of the retail
> > > cleanup logic in RelationDestroyRelation ...
> >
> > I think that idea might have a lot of merit, but I haven't studied it closely.
>
> It just occurred to me that one advantage of this would be that you
> could see how much memory was being used by each relcache entry using
> MemoryContextStats(), which seems super-appealing.  In fact, what
> about getting rid of all allocations in CacheMemoryContext itself in
> favor of some more specific context in each case?  That would make it
> a lot clearer where to look for leaks -- or efficiencies.

But it might also make it frustrating to look at memory context dumps -
we'd suddenly have many many more memory context lines we'd displayed,
right? Wouldn't that often make the dump extremely long?

To be clear, I think the idea has merit. Just want to raise the above
point.

Greetings,

Andres Freund

12