Thread: Re: [previously on HACKERS] "Compacting" a relation
On Mon, 2007-02-05 at 11:55 +0000, Simon Riggs wrote:
> On Sat, 2007-02-03 at 22:11 -0500, Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Peter Eisentraut <peter_e@gmx.net> writes:
> > > > vacuumlazy.c contains a hint "Consider compacting this relation" but AFAICT,
> > > > there is no indication anywhere how "compacting" is supposed to be achieved.
> > > > I guess this means VACUUM FULL or CLUSTER, but I don't think the hint can be
> > > > processed effectively by a user.
> > >
> > > So change it ...
> >
> > New message is:
> >
> > errhint("Consider using VACUUM FULL on this relation or increasing the configuration parameter
\"max_fsm_pages\".")));
> >
>
> The change of wording may be appropriate, but it is triggered when
>
> if (vacrelstats->tot_free_pages > MaxFSMPages)
>
> So if you VACUUM a 15+GB table and it has only 1% freespace then it will
> still generate this message. Hopefully you'd agree that the message
> would be inappropriate in that case.
>
> It's also inappropriate because this message is generated *prior* to
> doing lazy_truncate_heap(), which could easily remove lots of empty
> pages anyhow. That might reduce it to less than MaxFSMPages anyhow, so
> it can currently be triggered in wholly inappropriate situations.
>
> So I suggest that we move this wording after lazy_truncate_heap() in
> lazy_vacuum_rel() *and* we alter the hint so that it only suggests
> VACUUM FULL if the table has 20% fragmentation, whatever its size.
>
> Happy to drop a patch for this, if people agree.
Enclose 2 versions:
v1 - move test and WARNING
v2 - move test and WARNING, plus adjust hint according to relation size
--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Simon Riggs wrote:
> On Mon, 2007-02-05 at 11:55 +0000, Simon Riggs wrote:
> > On Sat, 2007-02-03 at 22:11 -0500, Bruce Momjian wrote:
> > > Tom Lane wrote:
> > > > Peter Eisentraut <peter_e@gmx.net> writes:
> > > > > vacuumlazy.c contains a hint "Consider compacting this relation" but AFAICT,
> > > > > there is no indication anywhere how "compacting" is supposed to be achieved.
> > > > > I guess this means VACUUM FULL or CLUSTER, but I don't think the hint can be
> > > > > processed effectively by a user.
> > > >
> > > > So change it ...
> > >
> > > New message is:
> > >
> > > errhint("Consider using VACUUM FULL on this relation or increasing the configuration parameter
\"max_fsm_pages\".")));
> > >
> >
> > The change of wording may be appropriate, but it is triggered when
> >
> > if (vacrelstats->tot_free_pages > MaxFSMPages)
> >
> > So if you VACUUM a 15+GB table and it has only 1% freespace then it will
> > still generate this message. Hopefully you'd agree that the message
> > would be inappropriate in that case.
> >
> > It's also inappropriate because this message is generated *prior* to
> > doing lazy_truncate_heap(), which could easily remove lots of empty
> > pages anyhow. That might reduce it to less than MaxFSMPages anyhow, so
> > it can currently be triggered in wholly inappropriate situations.
> >
> > So I suggest that we move this wording after lazy_truncate_heap() in
> > lazy_vacuum_rel() *and* we alter the hint so that it only suggests
> > VACUUM FULL if the table has 20% fragmentation, whatever its size.
> >
> > Happy to drop a patch for this, if people agree.
>
> Enclose 2 versions:
> v1 - move test and WARNING
> v2 - move test and WARNING, plus adjust hint according to relation size
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>
[ Attachment, skipping... ]
[ Attachment, skipping... ]
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
I applied the optional VACUUM FULL version, but modified to code to say
20% rather than a factor of 5, attached.
---------------------------------------------------------------------------
Simon Riggs wrote:
> On Mon, 2007-02-05 at 11:55 +0000, Simon Riggs wrote:
> > On Sat, 2007-02-03 at 22:11 -0500, Bruce Momjian wrote:
> > > Tom Lane wrote:
> > > > Peter Eisentraut <peter_e@gmx.net> writes:
> > > > > vacuumlazy.c contains a hint "Consider compacting this relation" but AFAICT,
> > > > > there is no indication anywhere how "compacting" is supposed to be achieved.
> > > > > I guess this means VACUUM FULL or CLUSTER, but I don't think the hint can be
> > > > > processed effectively by a user.
> > > >
> > > > So change it ...
> > >
> > > New message is:
> > >
> > > errhint("Consider using VACUUM FULL on this relation or increasing the configuration parameter
\"max_fsm_pages\".")));
> > >
> >
> > The change of wording may be appropriate, but it is triggered when
> >
> > if (vacrelstats->tot_free_pages > MaxFSMPages)
> >
> > So if you VACUUM a 15+GB table and it has only 1% freespace then it will
> > still generate this message. Hopefully you'd agree that the message
> > would be inappropriate in that case.
> >
> > It's also inappropriate because this message is generated *prior* to
> > doing lazy_truncate_heap(), which could easily remove lots of empty
> > pages anyhow. That might reduce it to less than MaxFSMPages anyhow, so
> > it can currently be triggered in wholly inappropriate situations.
> >
> > So I suggest that we move this wording after lazy_truncate_heap() in
> > lazy_vacuum_rel() *and* we alter the hint so that it only suggests
> > VACUUM FULL if the table has 20% fragmentation, whatever its size.
> >
> > Happy to drop a patch for this, if people agree.
>
> Enclose 2 versions:
> v1 - move test and WARNING
> v2 - move test and WARNING, plus adjust hint according to relation size
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>
[ Attachment, skipping... ]
[ Attachment, skipping... ]
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/commands/vacuumlazy.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuumlazy.c,v
retrieving revision 1.83
diff -c -c -r1.83 vacuumlazy.c
*** src/backend/commands/vacuumlazy.c 4 Feb 2007 03:10:55 -0000 1.83
--- src/backend/commands/vacuumlazy.c 21 Feb 2007 22:13:59 -0000
***************
*** 180,185 ****
--- 180,195 ----
/* Update shared free space map with final free space info */
lazy_update_fsm(onerel, vacrelstats);
+ if (vacrelstats->tot_free_pages > MaxFSMPages)
+ ereport(WARNING,
+ (errmsg("relation \"%s.%s\" contains more than \"max_fsm_pages\" pages with useful free space",
+ get_namespace_name(RelationGetNamespace(onerel)),
+ RelationGetRelationName(onerel)),
+ errhint("Consider%sincreasing the configuration parameter \"max_fsm_pages\".",
+ /* Only suggest VACUUM FULL if 20% free */
+ (vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20
+ ? " using VACUUM FULL on this relation or ": " "))));
+
/* Update statistics in pg_class */
vac_update_relstats(RelationGetRelid(onerel),
vacrelstats->rel_pages,
***************
*** 507,519 ****
vacrelstats->tot_free_pages,
empty_pages,
pg_rusage_show(&ru0))));
-
- if (vacrelstats->tot_free_pages > MaxFSMPages)
- ereport(WARNING,
- (errmsg("relation \"%s.%s\" contains more than \"max_fsm_pages\" pages with useful free space",
- get_namespace_name(RelationGetNamespace(onerel)),
- relname),
- errhint("Consider using VACUUM FULL on this relation or increasing the configuration parameter
\"max_fsm_pages\".")));
}
--- 517,522 ----
Bruce Momjian wrote:
>
> I applied the optional VACUUM FULL version, but modified to code to say
> 20% rather than a factor of 5, attached.
String construction does not work well with translations; please
reformulate this.
> + errhint("Consider%sincreasing the configuration parameter \"max_fsm_pages\".",
> + /* Only suggest VACUUM FULL if 20% free */
> + (vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20
> + ? " using VACUUM FULL on this relation or ": " "))));
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote:
> Bruce Momjian wrote:
> >
> > I applied the optional VACUUM FULL version, but modified to code to say
> > 20% rather than a factor of 5, attached.
>
> String construction does not work well with translations; please
> reformulate this.
>
> > + errhint("Consider%sincreasing the configuration parameter \"max_fsm_pages\".",
> > + /* Only suggest VACUUM FULL if 20% free */
> > + (vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20
> > + ? " using VACUUM FULL on this relation or ": " "))));
OK, updated.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/commands/vacuumlazy.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuumlazy.c,v
retrieving revision 1.84
diff -c -c -r1.84 vacuumlazy.c
*** src/backend/commands/vacuumlazy.c 21 Feb 2007 22:15:21 -0000 1.84
--- src/backend/commands/vacuumlazy.c 21 Feb 2007 22:41:55 -0000
***************
*** 185,194 ****
(errmsg("relation \"%s.%s\" contains more than \"max_fsm_pages\" pages with useful free space",
get_namespace_name(RelationGetNamespace(onerel)),
RelationGetRelationName(onerel)),
! errhint("Consider%sincreasing the configuration parameter \"max_fsm_pages\".",
! /* Only suggest VACUUM FULL if 20% free */
! (vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20
! ? " using VACUUM FULL on this relation or ": " "))));
/* Update statistics in pg_class */
vac_update_relstats(RelationGetRelid(onerel),
--- 185,194 ----
(errmsg("relation \"%s.%s\" contains more than \"max_fsm_pages\" pages with useful free space",
get_namespace_name(RelationGetNamespace(onerel)),
RelationGetRelationName(onerel)),
! errhint((vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20 ?
! /* Only suggest VACUUM FULL if 20% free */
! "Consider using VACUUM FULL on this relation or increasing the configuration parameter
\"max_fsm_pages\".":
! "Consider increasing the configuration parameter \"max_fsm_pages\"."))));
/* Update statistics in pg_class */
vac_update_relstats(RelationGetRelid(onerel),
Bruce Momjian wrote:
> Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> > >
> > > I applied the optional VACUUM FULL version, but modified to code to say
> > > 20% rather than a factor of 5, attached.
> >
> > String construction does not work well with translations; please
> > reformulate this.
> >
> > > + errhint("Consider%sincreasing the configuration parameter \"max_fsm_pages\".",
> > > + /* Only suggest VACUUM FULL if 20% free */
> > > + (vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20
> > > + ? " using VACUUM FULL on this relation or ": " "))));
>
> OK, updated.
Thanks :-)
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
On Wed, 2007-02-21 at 21:28 -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > Alvaro Herrera wrote:
> > > Bruce Momjian wrote:
> > > >
> > > > I applied the optional VACUUM FULL version, but modified to code to say
> > > > 20% rather than a factor of 5, attached.
> > >
> > > String construction does not work well with translations; please
> > > reformulate this.
> > >
> > > > + errhint("Consider%sincreasing the configuration parameter \"max_fsm_pages\".",
> > > > + /* Only suggest VACUUM FULL if 20% free */
> > > > + (vacrelstats->tot_free_pages > vacrelstats->rel_pages * 0.20
> > > > + ? " using VACUUM FULL on this relation or ": " "))));
> >
> > OK, updated.
>
> Thanks :-)
Alvaro: point noted for future. Bruce: many thanks.
--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com