Re: [HACKERS] Memory leak in vac_update_relstats ?
Tom Lane wrote: Gregory Stark [EMAIL PROTECTED] 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. I played with such a beast last winter. It suits the parser particularly well, it does a lot of tiny allocations that are all freed together, and palloc is at the top of the CPU profile. My implementation was a simple stack-like allocator, with a no-op pfree. I got carried away with other stuff, but I remember that one difficulty was using a different memory context for the parser because of the hack in PreventTransactionChain that checked if a piece of memory was allocated in QueryContext. I'm glad it's gone. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[HACKERS] Memory leak in vac_update_relstats ?
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
Re: [HACKERS] Memory leak in vac_update_relstats ?
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
Re: [HACKERS] Memory leak in vac_update_relstats ?
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
Re: [HACKERS] Memory leak in vac_update_relstats ?
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
Re: [HACKERS] Memory leak in vac_update_relstats ?
NikhilS [EMAIL PROTECTED] 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 [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] Memory leak in vac_update_relstats ?
On 7/20/07, Heikki Linnakangas [EMAIL PROTECTED] 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
Re: [HACKERS] Memory leak in vac_update_relstats ?
Pavan Deolasee [EMAIL PROTECTED] writes: On 7/20/07, Heikki Linnakangas [EMAIL PROTECTED] 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
Re: [HACKERS] Memory leak in vac_update_relstats ?
Tom Lane [EMAIL PROTECTED] 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
Re: [HACKERS] Memory leak in vac_update_relstats ?
Gregory Stark [EMAIL PROTECTED] 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
Re: [HACKERS] Memory leak in vac_update_relstats ?
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
