Re: [HACKERS] Memory leak in GIN index build
I had the possibility to perform tests on 9.5, and can confirm the memory leak I was seeing is solved with the patch (and that's great :) ) Regards Marc On 18/04/2016 17:53, Julien Rouhaud wrote: > On 18/04/2016 16:33, Tom Lane wrote: >> I poked at this over the weekend, and got more unhappy the more I poked. >> Aside from the memory leakage issue, there are multiple coding-rule >> violations besides the one you noted about scope of the critical sections. >> One example is that in the page-split path for an inner page, we modify >> the contents of childbuf long before getting into the critical section. >> The number of out-of-date comments was depressingly large as well. >> >> I ended up deciding that the most reasonable fix was along the lines of >> my first alternative above. In the attached draft patches, the >> "placeToPage" method is split into two methods, "beginPlaceToPage" and >> "execPlaceToPage", where the second method is what's supposed to happen >> inside the critical section for a non-page-splitting update. Nothing >> that's done inside beginPlaceToPage is critical. >> >> I've tested this to the extent of verifying that it passes make >> check-world and stops the memory leak in your test case, but it could use >> more eyeballs on it. >> > Thanks! I also started working on it but it was very far from being > complete (and already much more ugly...). > > I couldn't find any problem in the patch. > > I wonder if asserting being in a critical section would be valuable in > the *execPlaceToPage functions, or that (leaf->walinfolen > 0) in > dataExecPlaceToPageLeaf(), since it's filled far from this function. > >> Attached are draft patches against HEAD and 9.5 (they're the same except >> for BufferGetPage calls). I think we'd also want to back-patch to 9.4, >> but that will take considerable additional work because of the XLOG API >> rewrite that happened in 9.5. I also noted that some of the coding-rule >> violations seem to be new in 9.5, so the problems may be less severe in >> 9.4 --- the memory leak definitely exists there, though. >> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in GIN index build
On 18/04/2016 16:33, Tom Lane wrote: > > I poked at this over the weekend, and got more unhappy the more I poked. > Aside from the memory leakage issue, there are multiple coding-rule > violations besides the one you noted about scope of the critical sections. > One example is that in the page-split path for an inner page, we modify > the contents of childbuf long before getting into the critical section. > The number of out-of-date comments was depressingly large as well. > > I ended up deciding that the most reasonable fix was along the lines of > my first alternative above. In the attached draft patches, the > "placeToPage" method is split into two methods, "beginPlaceToPage" and > "execPlaceToPage", where the second method is what's supposed to happen > inside the critical section for a non-page-splitting update. Nothing > that's done inside beginPlaceToPage is critical. > > I've tested this to the extent of verifying that it passes make > check-world and stops the memory leak in your test case, but it could use > more eyeballs on it. > Thanks! I also started working on it but it was very far from being complete (and already much more ugly...). I couldn't find any problem in the patch. I wonder if asserting being in a critical section would be valuable in the *execPlaceToPage functions, or that (leaf->walinfolen > 0) in dataExecPlaceToPageLeaf(), since it's filled far from this function. > Attached are draft patches against HEAD and 9.5 (they're the same except > for BufferGetPage calls). I think we'd also want to back-patch to 9.4, > but that will take considerable additional work because of the XLOG API > rewrite that happened in 9.5. I also noted that some of the coding-rule > violations seem to be new in 9.5, so the problems may be less severe in > 9.4 --- the memory leak definitely exists there, though. > -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in GIN index build
Julien Rouhaud writes:
> On 16/04/2016 20:45, Tom Lane wrote:
>> I think this needs to be redesigned so that the critical section and WAL
>> insertion calls all happen within a single straight-line piece of code.
>>
>> We could try making that place be ginPlaceToPage(). So far as
>> registerLeafRecompressWALData is concerned, that does not look that hard;
>> it could palloc and fill the WAL-data buffer the same as it's doing now,
>> then pass it back up to be registered (and eventually pfreed) in
>> ginPlaceToPage. However, that would also mean postponing the call of
>> dataPlaceToPageLeafRecompress(), which seems much messier. I assume
>> the data it's working from is mostly in the tmpCtx that
>> dataPlaceToPageLeaf wants to free before returning. Maybe we could
>> move creation/destruction of the tmpCtx out to ginPlaceToPage, as well?
>>
>> The other line of thought is to move the WAL work that ginPlaceToPage
>> does down into the subroutines. That would probably result in some
>> duplication of code, but it might end up being a cleaner solution.
> I'm not really confident, but I'll give a try. Probably with your
> second solution which looks easier.
I poked at this over the weekend, and got more unhappy the more I poked.
Aside from the memory leakage issue, there are multiple coding-rule
violations besides the one you noted about scope of the critical sections.
One example is that in the page-split path for an inner page, we modify
the contents of childbuf long before getting into the critical section.
The number of out-of-date comments was depressingly large as well.
I ended up deciding that the most reasonable fix was along the lines of
my first alternative above. In the attached draft patches, the
"placeToPage" method is split into two methods, "beginPlaceToPage" and
"execPlaceToPage", where the second method is what's supposed to happen
inside the critical section for a non-page-splitting update. Nothing
that's done inside beginPlaceToPage is critical.
I've tested this to the extent of verifying that it passes make
check-world and stops the memory leak in your test case, but it could use
more eyeballs on it.
Attached are draft patches against HEAD and 9.5 (they're the same except
for BufferGetPage calls). I think we'd also want to back-patch to 9.4,
but that will take considerable additional work because of the XLOG API
rewrite that happened in 9.5. I also noted that some of the coding-rule
violations seem to be new in 9.5, so the problems may be less severe in
9.4 --- the memory leak definitely exists there, though.
regards, tom lane
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index e593b2b..fd36ecf 100644
*** a/src/backend/access/gin/ginbtree.c
--- b/src/backend/access/gin/ginbtree.c
***
*** 17,22
--- 17,23
#include "access/gin_private.h"
#include "access/xloginsert.h"
#include "miscadmin.h"
+ #include "utils/memutils.h"
#include "utils/rel.h"
static void ginFindParents(GinBtree btree, GinBtreeStack *stack);
*** ginFindParents(GinBtree btree, GinBtreeS
*** 312,326
* Insert a new item to a page.
*
* Returns true if the insertion was finished. On false, the page was split and
! * the parent needs to be updated. (a root split returns true as it doesn't
! * need any further action by the caller to complete)
*
* When inserting a downlink to an internal page, 'childbuf' contains the
* child page that was split. Its GIN_INCOMPLETE_SPLIT flag will be cleared
! * atomically with the insert. Also, the existing item at the given location
! * is updated to point to 'updateblkno'.
*
* stack->buffer is locked on entry, and is kept locked.
*/
static bool
ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
--- 313,328
* Insert a new item to a page.
*
* Returns true if the insertion was finished. On false, the page was split and
! * the parent needs to be updated. (A root split returns true as it doesn't
! * need any further action by the caller to complete.)
*
* When inserting a downlink to an internal page, 'childbuf' contains the
* child page that was split. Its GIN_INCOMPLETE_SPLIT flag will be cleared
! * atomically with the insert. Also, the existing item at offset stack->off
! * in the target page is updated to point to updateblkno.
*
* stack->buffer is locked on entry, and is kept locked.
+ * Likewise for childbuf, if given.
*/
static bool
ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
*** ginPlaceToPage(GinBtree btree, GinBtreeS
*** 329,339
--- 331,358
{
Page page = BufferGetPage(stack->buffer, NULL, NULL,
BGP_NO_SNAPSHOT_TEST);
+ bool result;
GinPlaceToPageRC rc;
uint16 xlflags = 0;
Page childpage = NULL;
Page newlpage = NULL,
newrpage = NULL;
+ void *ptp_workspace = NULL;
+ MemoryContext tmpCxt;
+ MemoryContext oldCxt;
+
Re: [HACKERS] Memory leak in GIN index build
On 16/04/2016 20:45, Tom Lane wrote: > Julien Rouhaud writes: > >> Also, in dataPlaceToPageLeaf() and ginVacuumPostingTreeLeaf(), shouldn't >> the START_CRIT_SECTION() calls be placed before the xlog code? > > Yeah, they should. Evidently somebody kluged it to avoid doing a palloc > inside a critical section, while ignoring every other rule about where to > place critical sections. (Maybe we should put an assert about being > within a critical section into XLogBeginInsert.) > After a quick test, it appears that at least log_smgrcreate() calls XLogBeginInsert() without being in a critical section, from the various DDL commands. > This code is pretty fundamentally broken, isn't it. elog() calls > inside a critical section? Really? Even worse, a MemoryContextDelete, > which hardly seems like something that should be assumed error-proof. > > Not to mention the unbelievable ugliness of a function that sometimes > returns with an open critical section (and WAL insertion started) and > sometimes doesn't. > > It kinda looks like this was hacked up without bothering to keep the > comment block in ginPlaceToPage in sync with reality, either. > > I think this needs to be redesigned so that the critical section and WAL > insertion calls all happen within a single straight-line piece of code. > > We could try making that place be ginPlaceToPage(). So far as > registerLeafRecompressWALData is concerned, that does not look that hard; > it could palloc and fill the WAL-data buffer the same as it's doing now, > then pass it back up to be registered (and eventually pfreed) in > ginPlaceToPage. However, that would also mean postponing the call of > dataPlaceToPageLeafRecompress(), which seems much messier. I assume > the data it's working from is mostly in the tmpCtx that > dataPlaceToPageLeaf wants to free before returning. Maybe we could > move creation/destruction of the tmpCtx out to ginPlaceToPage, as well? > > The other line of thought is to move the WAL work that ginPlaceToPage > does down into the subroutines. That would probably result in some > duplication of code, but it might end up being a cleaner solution. > > Anyway, I think memory leakage is just the start of what's broken here, > and fixing it won't be a very small patch. > I'm not really confident, but I'll give a try. Probably with your second solution which looks easier. Any pointer is welcome, unless someone more familiar with this code wants to take it. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in GIN index build
Julien Rouhaud writes: > After some digging, the leak comes from walbufbegin palloc in > registerLeafRecompressWALData(). > IIUC, walbufbegin isn't pfree-d and can't be before XLogInsert() is > called, which happens in ginPlaceToPage(). Hmm. > I don't see a simple way to fix that. My first idea would be to change > GinBtreeData's placeToPage (and all other needed) functions signature to > pass a pointer to pfree in ginPlaceToPage() if needed, but it doesn't > really seem appealing. In any case, I'd be happy to work on a patch if > needed. > Also, in dataPlaceToPageLeaf() and ginVacuumPostingTreeLeaf(), shouldn't > the START_CRIT_SECTION() calls be placed before the xlog code? Yeah, they should. Evidently somebody kluged it to avoid doing a palloc inside a critical section, while ignoring every other rule about where to place critical sections. (Maybe we should put an assert about being within a critical section into XLogBeginInsert.) This code is pretty fundamentally broken, isn't it. elog() calls inside a critical section? Really? Even worse, a MemoryContextDelete, which hardly seems like something that should be assumed error-proof. Not to mention the unbelievable ugliness of a function that sometimes returns with an open critical section (and WAL insertion started) and sometimes doesn't. It kinda looks like this was hacked up without bothering to keep the comment block in ginPlaceToPage in sync with reality, either. I think this needs to be redesigned so that the critical section and WAL insertion calls all happen within a single straight-line piece of code. We could try making that place be ginPlaceToPage(). So far as registerLeafRecompressWALData is concerned, that does not look that hard; it could palloc and fill the WAL-data buffer the same as it's doing now, then pass it back up to be registered (and eventually pfreed) in ginPlaceToPage. However, that would also mean postponing the call of dataPlaceToPageLeafRecompress(), which seems much messier. I assume the data it's working from is mostly in the tmpCtx that dataPlaceToPageLeaf wants to free before returning. Maybe we could move creation/destruction of the tmpCtx out to ginPlaceToPage, as well? The other line of thought is to move the WAL work that ginPlaceToPage does down into the subroutines. That would probably result in some duplication of code, but it might end up being a cleaner solution. Anyway, I think memory leakage is just the start of what's broken here, and fixing it won't be a very small patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Memory leak in GIN index build
Hello, Another colleague provided a report of memory leak, during a GIN index build. Test case to reproduce the attached (need to create a gin index on the val column after loading). Sorry, it generates a 24GB table, and memory start leaking with a 1GB maintenance_work_mem after reaching 8 or 9 times the m_w_m setting, leading to ~ 9GB used in Gin build temporary context. After some digging, the leak comes from walbufbegin palloc in registerLeafRecompressWALData(). IIUC, walbufbegin isn't pfree-d and can't be before XLogInsert() is called, which happens in ginPlaceToPage(). I don't see a simple way to fix that. My first idea would be to change GinBtreeData's placeToPage (and all other needed) functions signature to pass a pointer to pfree in ginPlaceToPage() if needed, but it doesn't really seem appealing. In any case, I'd be happy to work on a patch if needed. Also, in dataPlaceToPageLeaf() and ginVacuumPostingTreeLeaf(), shouldn't the START_CRIT_SECTION() calls be placed before the xlog code? Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org generate_data.pl Description: Perl program -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] memory leak in GIN
Jeff Janes writes: > I bisected it down to: > d88976cfa1302e8dccdcbfe55e9e29faee8c0cdf is the first bad commit > commit d88976cfa1302e8dccdcbfe55e9e29faee8c0cdf > Author: Heikki Linnakangas > Date: Wed Feb 4 17:40:25 2015 +0200 > Use a separate memory context for GIN scan keys. Yeah, I had come to the same conclusion. The leak comes from removing this bit from ginFreeScanKeys(): - if (entry->list) - pfree(entry->list); Heikki evidently supposed that entry->list would be allocated in the so->keyCtx, but as things stand, it is not: it gets allocated in the query-lifespan context, so this change causes a leak of potentially several kB per ginrescan cycle. And the test query does about 12 of those. I think it would likely be a good thing to fix things so that that assumption actually holds, but I'm not familiar enough with this code to decide what's the best way to do that. (Basically, the question is "how much of startScanEntry() ought to run with keyCtx as current memory context?") As a short-term fix I plan to reinstall the above-cited pfree. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] memory leak in GIN
On Fri, Mar 11, 2016 at 11:40 PM, Jaime Casanova wrote: > Hi, > > On the spanish list, Felipe de Jesús Molina Bravo, reported a few days > back that a query that worked well in 9.4 consume all memory in 9.5. > With the self contained test he provided us i reproduced the problem > in 9.5 and 9.6dev. > > To test, execute: > > pba.sql -- to create the tables and populate > query_crash.sql -- this will consume all your memory and crash your > server eventually > > If you drop the GIN indexes, the problem disappear. > > I used valgrind to try to hunt the memory leak, attached the resulting > log showing the backend that executed the query. And from the little a > could say from the stack trace valgrind showed, the problem is around > ginPostingListDecodeAllSegments() but i don't see any modifications > there. I bisected it down to: d88976cfa1302e8dccdcbfe55e9e29faee8c0cdf is the first bad commit commit d88976cfa1302e8dccdcbfe55e9e29faee8c0cdf Author: Heikki Linnakangas Date: Wed Feb 4 17:40:25 2015 +0200 Use a separate memory context for GIN scan keys. It was getting tedious to track and release all the different things that form a scan key. We were leaking at least the queryCategories array, and possibly more, on a rescan. That was visible if a GIN index was used in a nested loop join. This also protects from leaks in extractQuery method. No backpatching, given the lack of complaints from the field. Maybe later, after this has received more field testing. I won't have a chance to do any further analysis for a while. Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
