At the very least, couldn't vc_vpinsert() double
vpl->vpl_num_pages whenever vpl->vpl_num_pages
needs to be expanded instead of expanding linearly
by PG_NPAGEDESC, or by the original 100?

Mike Mascari
(mascarim@yahoo.com)

--- Hiroshi Inoue <Inoue@tpf.co.jp> wrote:
> Hi all,
> 
> I found the following comment in utils/mmgr/aset.c.
> The high memory usage of big vacuum is probably
> caused by this
> change.
> Calling repalloc() many times with its size
> parameter increasing
> would need large amount of memory.
> 
> Should vacuum call realloc() directly ?
> Or should AllocSet..() be changed ?
> 
> Comments ?
> 
>  * NOTE:
>  *      This is a new (Feb. 05, 1999) implementation
> of the allocation set
>  *      routines. AllocSet...() does not use
> OrderedSet...() any more.
>  *      Instead it manages allocations in a block
> pool by itself, combining
>  *      many small allocations in a few bigger
> blocks. AllocSetFree() does
>  *      never free() memory really. It just add's
> the free'd area to some
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  *      list for later reuse by AllocSetAlloc(). All
> memory blocks are
> free()'d
> 
> Regards.
> 
> Hiroshi Inoue
> Inoue@tpf.co.jp

> > *** vacuum.c.orig	Sat Jul  3 09:32:40 1999
> > --- vacuum.c	Thu Aug 19 17:34:18 1999
> > ***************
> > *** 2519,2530 ****
> >   static void
> >   vc_vpinsert(VPageList vpl, VPageDescr vpnew)
> >   {
> >
> >   	/* allocate a VPageDescr entry if needed */
> >   	if (vpl->vpl_num_pages == 0)
> > ! 		vpl->vpl_pagedesc = (VPageDescr *) palloc(100
> *
> > sizeof(VPageDescr));
> > ! 	else if (vpl->vpl_num_pages % 100 == 0)
> > ! 		vpl->vpl_pagedesc = (VPageDescr *)
> > repalloc(vpl->vpl_pagedesc, (vpl->vpl_num_pages +
> 100) *
> > sizeof(VPageDescr));
> >   	vpl->vpl_pagedesc[vpl->vpl_num_pages] = vpnew;
> >   	(vpl->vpl_num_pages)++;
> >
> > --- 2519,2531 ----
> >   static void
> >   vc_vpinsert(VPageList vpl, VPageDescr vpnew)
> >   {
> > + #define PG_NPAGEDESC 1000
> >
> >   	/* allocate a VPageDescr entry if needed */
> >   	if (vpl->vpl_num_pages == 0)
> > ! 		vpl->vpl_pagedesc = (VPageDescr *)
> > palloc(PG_NPAGEDESC * sizeof(VPageDescr));
> > ! 	else if (vpl->vpl_num_pages % PG_NPAGEDESC ==
> 0)
> > ! 		vpl->vpl_pagedesc = (VPageDescr *)
> > repalloc(vpl->vpl_pagedesc, (vpl->vpl_num_pages +
> PG_NPAGEDESC) *
> > sizeof(VPageDescr));
> >   	vpl->vpl_pagedesc[vpl->vpl_num_pages] = vpnew;
> >   	(vpl->vpl_num_pages)++;
> >

__________________________________________________
Do You Yahoo!?
Bid and sell for free at http://auctions.yahoo.com


Fri, 20 Aug 1999 02:41:19 -0700 (PDT)
Mike Mascari <mascarim@yahoo.com>
RE: [HACKERS] vacuum process size 
Mike, 

> At the very least, couldn't vc_vpinsert() double
> vpl->vpl_num_pages whenever vpl->vpl_num_pages
> needs to be expanded instead of expanding linearly
> by PG_NPAGEDESC, or by the original 100?

I have tested your idea and found even more improved memory usage
(86MB vs. 43MB). Standard vacuum consumes as much as 478MB memory with
deleting 5000000 tuples that would not be acceptable for most
configurations. I think we should fix this as soon as possible.  If
there's no objection, I will commit included patches to the stable
tree (seems Tom has more aggressive idea, so I'll leave the current
tree as it is).
---
Tatsuo Ishii
-------------------------------------------------------------------
*** vacuum.c.orig	Sat Jul  3 09:32:40 1999
--- vacuum.c	Tue Aug 24 10:08:43 1999
***************
*** 2519,2530 ****
  static void
  vc_vpinsert(VPageList vpl, VPageDescr vpnew)
  {
  
  	/* allocate a VPageDescr entry if needed */
  	if (vpl->vpl_num_pages == 0)
! 		vpl->vpl_pagedesc = (VPageDescr *) palloc(100 * sizeof(VPageDescr));
! 	else if (vpl->vpl_num_pages % 100 == 0)
! 		vpl->vpl_pagedesc = (VPageDescr *) repalloc(vpl->vpl_pagedesc, (vpl->vpl_num_pages + 100) * sizeof(VPageDescr));
  	vpl->vpl_pagedesc[vpl->vpl_num_pages] = vpnew;
  	(vpl->vpl_num_pages)++;
  
--- 2519,2538 ----
  static void
  vc_vpinsert(VPageList vpl, VPageDescr vpnew)
  {
+ #define PG_NPAGEDESC 1024
+ static uint num_pages;
  
  	/* allocate a VPageDescr entry if needed */
  	if (vpl->vpl_num_pages == 0)
! 	{
! 		vpl->vpl_pagedesc = (VPageDescr *) palloc(PG_NPAGEDESC * sizeof(VPageDescr));
! 		num_pages = PG_NPAGEDESC;
! 	}
! 	else if (vpl->vpl_num_pages >= num_pages)
! 	{
! 		num_pages *= 2;
! 		vpl->vpl_pagedesc = (VPageDescr *) repalloc(vpl->vpl_pagedesc, num_pages * sizeof(VPageDescr));
! 	}
  	vpl->vpl_pagedesc[vpl->vpl_num_pages] = vpnew;
  	(vpl->vpl_num_pages)++;
  

Tue, 24 Aug 1999 10:12:37 +0900
Tatsuo Ishii <t-ishii@sra.co.jp>
Re: [HACKERS] vacuum process size 
> At the very least, couldn't vc_vpinsert() double
> vpl->vpl_num_pages whenever vpl->vpl_num_pages
> needs to be expanded instead of expanding linearly
> by PG_NPAGEDESC, or by the original 100?

This seems like a good idea.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Mon, 23 Aug 1999 23:59:51 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] vacuum process size
Tatsuo Ishii <t-ishii@sra.co.jp> writes:
> I have tested your idea and found even more improved memory usage
> (86MB vs. 43MB). Standard vacuum consumes as much as 478MB memory with
> deleting 5000000 tuples that would not be acceptable for most
> configurations. I think we should fix this as soon as possible.  If
> there's no objection, I will commit included patches to the stable
> tree (seems Tom has more aggressive idea, so I'll leave the current
> tree as it is).

No, please make the change in current as well.  I was thinking about
tweaking aset.c to be smarter about releasing large chunks, but in any
case having the doubling behavior at the request point will be a big
improvement.

I do not like your patch as given, however.  By using a static variable
you are assuming that there is only one active VPageList at a time.
It looks to me like there are at least two --- and there is no reason
to think they'd be the same size.

You need to add a num_pages field to the VPageList struct, not use
a static.

			regards, tom lane

Tue, 24 Aug 1999 11:05:28 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] vacuum process size 
I have been looking some more at the vacuum-process-size issue, and
I am having a hard time understanding why the VPageList data structure
is the critical one.  As far as I can see, there should be at most one
pointer in it for each disk page of the relation.  OK, you were
vacuuming a table with something like a quarter million pages, so
the end size of the VPageList would have been something like a megabyte,
and given the inefficient usage of repalloc() in the original code,
a lot more space than that would have been wasted as the list grew.
So doubling the array size at each step is a good change.

But there are a lot more tuples than pages in most relations.

I see two lists with per-tuple data in vacuum.c, "vtlinks" in
vc_scanheap and "vtmove" in vc_rpfheap, that are both being grown with
essentially the same technique of repalloc() after every N entries.
I'm not entirely clear on how many tuples get put into each of these
lists, but it sure seems like in ordinary circumstances they'd be much
bigger space hogs than any of the three VPageList lists.

I recommend going to a doubling approach for each of these lists as
well as for VPageList.

There is a fourth usage of repalloc with the same method, for "ioid"
in vc_getindices.  This only gets one entry per index on the current
relation, so it's unlikely to be worth changing on its own merit.
But it might be worth building a single subroutine that expands a
growable list of entries (taking sizeof() each entry as a parameter)
and applying it in all four places.

			regards, tom lane

Tue, 24 Aug 1999 12:20:22 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] vacuum process size 
Then <tgl@sss.pgh.pa.us> spoke up and said:
> So doubling the array size at each step is a good change.
> 
> But there are a lot more tuples than pages in most relations.
> 
> I see two lists with per-tuple data in vacuum.c, "vtlinks" in
> vc_scanheap and "vtmove" in vc_rpfheap, that are both being grown with
> essentially the same technique of repalloc() after every N entries.
> I'm not entirely clear on how many tuples get put into each of these
> lists, but it sure seems like in ordinary circumstances they'd be much
> bigger space hogs than any of the three VPageList lists.
> 
> I recommend going to a doubling approach for each of these lists as
> well as for VPageList.

Question: is there reliable information in pg_statistics (or other
system tables) which can be used to make a reasonable estimate for the
sizes of these structures before initial allocation?  Certainly the
file size can be gotten from a stat (some portability issues, sparse
file issues).


-- 
=====================================================================
| JAVA must have been developed in the wilds of West Virginia.      |
| After all, why else would it support only single inheritance??    |
=====================================================================
| Finger geek@cmu.edu for my public key.                            |
=====================================================================
24 Aug 1999 13:01:12 -0400
Brian E Gallew <geek+@cmu.edu>
Re: [HACKERS] vacuum process size 
>> If there's no objection, I will commit included patches to the stable
>> tree (seems Tom has more aggressive idea, so I'll leave the current
>> tree as it is).

> No, please make the change in current as well.  I was thinking about
> tweaking aset.c to be smarter about releasing large chunks, but in any
> case having the doubling behavior at the request point will be a big
> improvement.

I have just committed changes into current (but not REL6_5) to make
aset.c smarter about giving back memory from large requests.  Basically,
for chunk sizes >= ALLOC_BIGCHUNK_LIMIT, pfree() does an actual free()
and repalloc() does an actual realloc().  There is no change in behavior
for smaller chunk sizes.  This should cap the amount of space that can
be wasted by aset.c while repalloc'ing a chunk larger and larger.

For lack of a better idea I set ALLOC_BIGCHUNK_LIMIT to 64K.  I don't
think it'd pay to make it very small, but I don't really know whether
this is a good choice or not.

It would still be a good idea to fix vacuum.c to double its repalloc
requests at each step, but Tatsuo was already working on that part
so I won't joggle his elbow...

			regards, tom lane

Tue, 24 Aug 1999 16:21:24 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] vacuum process size 
Brian E Gallew <geek+@cmu.edu> writes:
> Question: is there reliable information in pg_statistics (or other
> system tables) which can be used to make a reasonable estimate for the
> sizes of these structures before initial allocation?  Certainly the
> file size can be gotten from a stat (some portability issues, sparse
> file issues).

pg_statistics would tell you what was found out by the last vacuum on
the table, if there ever was one.  Dunno how reliable you want to
consider that to be.  stat() would provide up-to-date info, but the
problem with it is that the total file size might be a drastic
overestimate of the number of pages that vacuum needs to put in these
lists.  There's not really much chance of getting a useful estimate from
the last vacuum run, either.  AFAICT what we are interested in is the
number of pages containing dead tuples, and by definition all of those
tuples will have died since the last vacuum...

On the whole, just fixing the memory management seems like the best bet.
We know how to do that, and it may benefit other things besides vacuum.

			regards, tom lane

Tue, 24 Aug 1999 16:51:58 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] vacuum process size 
>I have just committed changes into current (but not REL6_5) to make

Just for a confirmation: I see REL6_5_PATCHES and REL6_5 Tag in the
CVS respository. I thought that REL6_5_PATCHES is the Tag for the 6.5
statble tree and would eventually become 6.5.2. If so, what is the
REL6_5 Tag? Or I totally miss the point?
--
Tatsuo Ishii


Wed, 25 Aug 1999 09:08:29 +0900
Tatsuo Ishii <t-ishii@sra.co.jp>
Re: [HACKERS] vacuum process size 
> >I have just committed changes into current (but not REL6_5) to make
> 
> Just for a confirmation: I see REL6_5_PATCHES and REL6_5 Tag in the
> CVS respository. I thought that REL6_5_PATCHES is the Tag for the 6.5
> statble tree and would eventually become 6.5.2. If so, what is the
> REL6_5 Tag? Or I totally miss the point?

REL6_5 was a mistake.
-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tue, 24 Aug 1999 20:22:16 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] vacuum process size
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Sent: Wednesday, August 25, 1999 1:20 AM
> To: t-ishii@sra.co.jp
> Cc: Mike Mascari; Hiroshi Inoue; pgsql-hackers@postgreSQL.org
> Subject: Re: [HACKERS] vacuum process size 
> 
> 
> I have been looking some more at the vacuum-process-size issue, and
> I am having a hard time understanding why the VPageList data structure
> is the critical one.  As far as I can see, there should be at most one
> pointer in it for each disk page of the relation.  OK, you were
> vacuuming a table with something like a quarter million pages, so
> the end size of the VPageList would have been something like a megabyte,
> and given the inefficient usage of repalloc() in the original code,
> a lot more space than that would have been wasted as the list grew.
> So doubling the array size at each step is a good change.
> 
> But there are a lot more tuples than pages in most relations.
> 
> I see two lists with per-tuple data in vacuum.c, "vtlinks" in
> vc_scanheap and "vtmove" in vc_rpfheap, that are both being grown with
> essentially the same technique of repalloc() after every N entries.
> I'm not entirely clear on how many tuples get put into each of these
> lists, but it sure seems like in ordinary circumstances they'd be much
> bigger space hogs than any of the three VPageList lists.
>

AFAIK,both vtlinks and vtmove are NULL if vacuum is executed
without concurrent transactions.
They won't be so big unless loooong concurrent transactions exist.
 
Regards.

Hiroshi Inoue
Inoue@tpf.co.jp

Wed, 25 Aug 1999 10:11:42 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: [HACKERS] vacuum process size 
Tatsuo Ishii <t-ishii@sra.co.jp> writes:
> Just for a confirmation: I see REL6_5_PATCHES and REL6_5 Tag in the
> CVS respository. I thought that REL6_5_PATCHES is the Tag for the 6.5
> statble tree and would eventually become 6.5.2. If so, what is the
> REL6_5 Tag? Or I totally miss the point?

Right, REL6_5_PATCHES is the 6.5.* branch.  REL6_5 is just a tag ---
that is, it's effectively a frozen snapshot of the 6.5 release,
not an evolvable branch.

I am not sure if Marc intends to continue this naming convention
in future, or if it was just a mistake to create REL6_5 as a tag
not a branch.  I don't see a whole lot of use for the frozen tag
myself...

			regards, tom lane

Wed, 25 Aug 1999 09:46:10 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] vacuum process size 
> Tatsuo Ishii <t-ishii@sra.co.jp> writes:
> > I have tested your idea and found even more improved memory usage
> > (86MB vs. 43MB). Standard vacuum consumes as much as 478MB memory with
> > deleting 5000000 tuples that would not be acceptable for most
> > configurations. I think we should fix this as soon as possible.  If
> > there's no objection, I will commit included patches to the stable
> > tree (seems Tom has more aggressive idea, so I'll leave the current
> > tree as it is).
> 
> No, please make the change in current as well.  I was thinking about
> tweaking aset.c to be smarter about releasing large chunks, but in any
> case having the doubling behavior at the request point will be a big
> improvement.
> 
> I do not like your patch as given, however.  By using a static variable
> you are assuming that there is only one active VPageList at a time.
> It looks to me like there are at least two --- and there is no reason
> to think they'd be the same size.
> 
> You need to add a num_pages field to the VPageList struct, not use
> a static.

Good point. I have committed new patches that do not use static
variables anymore to both REL6_5_PATCHES and current tree.

Modified files: backend/commands/vacuum.c and
include/commands/vacuum.h.
---
Tatsuo Ishii

Wed, 25 Aug 1999 22:50:39 +0900
Tatsuo Ishii <t-ishii@sra.co.jp>
Re: [HACKERS] vacuum process size 
On Wed, 25 Aug 1999, Tom Lane wrote:

> Tatsuo Ishii <t-ishii@sra.co.jp> writes:
> > Just for a confirmation: I see REL6_5_PATCHES and REL6_5 Tag in the
> > CVS respository. I thought that REL6_5_PATCHES is the Tag for the 6.5
> > statble tree and would eventually become 6.5.2. If so, what is the
> > REL6_5 Tag? Or I totally miss the point?
> 
> Right, REL6_5_PATCHES is the 6.5.* branch.  REL6_5 is just a tag ---
> that is, it's effectively a frozen snapshot of the 6.5 release,
> not an evolvable branch.
> 
> I am not sure if Marc intends to continue this naming convention
> in future, or if it was just a mistake to create REL6_5 as a tag
> not a branch.  I don't see a whole lot of use for the frozen tag
> myself...

I like the frozen tag myself, since, in the future, if we need to create a
quick tar ball of what things looked like at that release (ie.
v6.5->v6.5.2 patch?), its easy to generate...

Actually, come to think of it...am going to try that out now...report back
in a bit...

Marc G. Fournier                   ICQ#7615664               IRC Nick: Scrappy
Systems Administrator @ hub.org 
primary: scrappy@hub.org           secondary: scrappy@{freebsd|postgresql}.org 


Wed, 25 Aug 1999 11:05:54 -0300 (ADT)
The Hermit Hacker <scrappy@hub.org>
Re: [HACKERS] vacuum process size 