Are we leaking memory in vac_update_relstats ?

/* Fetch a copy of the tuple to scribble on */
ctup = SearchSysCacheCopy(RELOID,
ObjectIdGetDatum(relid),
0, 0, 0);

This copy is not subsequently freed in the function.

Thanks,
Pavan


--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

Pavan Deolasee wrote:
> Are we leaking memory in vac_update_relstats ?
>
> /* Fetch a copy of the tuple to scribble on */
> ctup = SearchSysCacheCopy(RELOID,
> ObjectIdGetDatum(relid),
> 0, 0, 0);
>
> This copy is not subsequently freed in the function.

It's palloc'd in the current memory context, so it's not serious. It'll
be freed at the end of the transaction, if not before that. That's the
beauty of memory contexts; no need to worry about small allocations like
that.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Hi,

>
> It's palloc'd in the current memory context, so it's not serious. It'll
> be freed at the end of the transaction, if not before that. That's the
> beauty of memory contexts; no need to worry about small allocations like
> that.


That's the beauty of memory contexts for small allocations. But because of
the 'convenience' of memory contexts we sometimes tend to not pay attention
to doing explicit pfrees. As a general rule I think allocations in
TopMemoryContext should be critically examined. I was bitten by this undue
bloat recently while developing some code and valgrind is not of much help
in such cases because of this very beauty of memory contexts .

Regards,
Nikhils

--
EnterpriseDB http://www.enterprisedb.com

Hi,

That's the beauty of memory contexts for small allocations. But because of
> the 'convenience' of memory contexts we sometimes tend to not pay attention
> to doing explicit pfrees. As a general rule I think allocations in
> TopMemoryContext should be critically examined. I was bitten by this undue
> bloat recently while developing some code and valgrind is not of much help
> in such cases because of this very beauty of memory contexts .



One specific case I want to mention here is hash_create(). For local hash
tables if HASH_CONTEXT is not specified, they get created in a context which
becomes a direct child of TopMemoryContext. Wouldn't it be a better idea to
create the table in CurrentMemoryContext?

If hash_destroy() is not explicitly invoked, this can cause a lot of bloat
especially if the intention was to use the hash table only for a while.

Regards,
Nikhils

Regards,
Nikhils


--
EnterpriseDB http://www.enterprisedb.com

NikhilS <nikkhils@gmail.com> writes:
> One specific case I want to mention here is hash_create(). For local hash
> tables if HASH_CONTEXT is not specified, they get created in a context which
> becomes a direct child of TopMemoryContext. Wouldn't it be a better idea to
> create the table in CurrentMemoryContext?

It works that way partly for historical reasons and mostly because the
vast majority of dynahash uses are for long-lived hash tables (a quick
search says that a default of CurrentMemoryContext would be correct for
only about one in ten of the current callers). I don't see any value in
changing it.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

On 7/20/07, Heikki Linnakangas <heikki@enterprisedb.com> wrote:
>
> Pavan Deolasee wrote:
> > Are we leaking memory in vac_update_relstats ?
> >
> > /* Fetch a copy of the tuple to scribble on */
> > ctup = SearchSysCacheCopy(RELOID,
> > ObjectIdGetDatum(relid),
> > 0, 0, 0);
> >
> > This copy is not subsequently freed in the function.
>
> It's palloc'd in the current memory context, so it's not serious. It'll
> be freed at the end of the transaction, if not before that. That's the
> beauty of memory contexts; no need to worry about small allocations like
> that.


Right. But may be for code completeness, we should add that
missing heap_freetuple.

Thanks,
Pavan




--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> On 7/20/07, Heikki Linnakangas <heikki@enterprisedb.com> wrote:
>> It's palloc'd in the current memory context, so it's not serious.

> Right. But may be for code completeness, we should add that
> missing heap_freetuple.

Personally I've been thinking of mounting an effort to get rid of
unnecessary pfree's wherever possible. Particularly in user-defined
functions, "cleaning up" at the end is a waste of code space and
cycles too, because they're typically called in contexts that are
going to be reset immediately afterward.

In the case of vac_update_relstats, it's called only once per
transaction, so there's certainly no point in being a neatnik.
Stuff you need to worry about is functions that might be called
many times in the same memory context.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

> Personally I've been thinking of mounting an effort to get rid of
> unnecessary pfree's wherever possible. Particularly in user-defined
> functions, "cleaning up" at the end is a waste of code space and
> cycles too, because they're typically called in contexts that are
> going to be reset immediately afterward.

It seems like the impact of this is self-limiting though. The worst-case is
going to be something which executes an extra pfree for every tuple. Or
perhaps one for every expression in a complex query involving lots of
expressions. Saving a few extra pfrees per tuple isn't really going to buy
many cpu cycles.

> In the case of vac_update_relstats, it's called only once per
> transaction, so there's certainly no point in being a neatnik.
> Stuff you need to worry about is functions that might be called
> many times in the same memory context.

Fwiw there are user-space functions that can't leak memory. I'm sure everyone
knows about btree operators, but pgsql also assumes that type input and output
functions don't leak memory too (or else assignment leaks memory in the
function scope memory context and in a loop...).

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Gregory Stark <stark@enterprisedb.com> writes:
> It seems like the impact of this is self-limiting though. The worst-case is
> going to be something which executes an extra pfree for every tuple. Or
> perhaps one for every expression in a complex query involving lots of
> expressions. Saving a few extra pfrees per tuple isn't really going to buy
> many cpu cycles.

I can't tell you how many profiles I've looked at in which palloc/pfree
were *the* dominant consumers of CPU cycles. I'm not sure how much
could be saved this particular way, but I wouldn't dismiss it as
uninteresting. I've actually thought about making short-term memory
contexts use a variant MemoryContext type in which pfree was a no-op and
palloc was simplified by not worrying at all about recycling space.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
> I've actually thought about making short-term memory
> contexts use a variant MemoryContext type in which pfree was a no-op and
> palloc was simplified by not worrying at all about recycling space.
>
>
>

That sounds like a good idea to me.

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match