Re: [HACKERS] Memory leak in GIN index build

2016-04-19 Thread Marc Cousin
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

2016-04-18 Thread Julien Rouhaud
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

2016-04-18 Thread Tom Lane
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;

Re: [HACKERS] Memory leak in GIN index build

2016-04-16 Thread Julien Rouhaud
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

2016-04-16 Thread Tom Lane
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


Re: [HACKERS] memory leak in GIN

2016-03-13 Thread Tom Lane
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

2016-03-13 Thread Jeff Janes
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