Обсуждение: Re: [GENERAL] A way to let Vacuum warn if FSM settings are

I never heard any discussion on whether this should be backpatched to
8.0.X.  Should it?

---------------------------------------------------------------------------

pgman wrote:
> Simon Riggs wrote:
> > On Mon, 2005-03-14 at 01:40 -0500, Tom Lane wrote:
> > > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > > Ron Mayer wrote:
> > > >> My reasoning why I thought the log file was more useful was
> > > >> that only an admin with access to the log files could really
> > > >> do anything about the message anyway.
> > >
> > > > The log file is useful, but I think showing the VACUUM user is _more_
> > > > useful than the log file.
> > >
> > > I think that reasoning is fundamentally unsound, because (a) a lot of
> > > people already do vacuuming via a cron job or autovacuum, and (b)
> > > autovacuum is definitely the wave of the future.  So it's foolish
> > > to design this messaging around the assumption that there will be
> > > a human attentive to the on-line output from VACUUM.  We should be
> > > ensuring that the message gets into the postmaster log --- whether
> > > it gets sent to the client is secondary.
> >
> > Personally, I prefer the postmaster log as the place for this.
> >
> > However, whilst vacuum exists as a separate command, there will be an
> > argument to return a message back to the person running it; we cannot
> > assume that people would be inattentive.
> >
> > Possibly the deciding factor should be whether autovacuum makes it fully
> > into becoming a special backend anytime soon, since in that case only
> > the log would remain as an option for reporting this message, in that
> > case.
> >
> > Can we have both?
>
> Sure.  It is very easy and in fact looks even cleaner than the original
> code because now the optional stuff is in its own function.
>
> Patch attached and applied.
>
> --
>   Bruce Momjian                        |  http://candle.pha.pa.us
>   pgman@candle.pha.pa.us               |  (610) 359-1001
>   +  If your life is a hard drive,     |  13 Roberts Road
>   +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

> Index: src/backend/storage/freespace/freespace.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/storage/freespace/freespace.c,v
> retrieving revision 1.38
> diff -c -c -r1.38 freespace.c
> *** src/backend/storage/freespace/freespace.c    12 Mar 2005 05:21:52 -0000    1.38
> --- src/backend/storage/freespace/freespace.c    14 Mar 2005 20:04:00 -0000
> ***************
> *** 221,226 ****
> --- 221,228 ----
>                                            * FSMHeader->relHash */
>
>
> + static void CheckFreeSpaceMapStatistics(int elevel, int numRels,
> +                         double needed);
>   static FSMRelation *lookup_fsm_rel(RelFileNode *rel);
>   static FSMRelation *create_fsm_rel(RelFileNode *rel);
>   static void delete_fsm_rel(FSMRelation *fsmrel);
> ***************
> *** 711,726 ****
>                errdetail("FSM size: %d relations + %d pages = %.0f kB shared memory.",
>                          MaxFSMRelations, MaxFSMPages,
>                          (double) FreeSpaceShmemSize() / 1024.0)));
> !
> !     if (numRels == MaxFSMRelations)
> !         ereport(NOTICE,
>               (errmsg("max_fsm_relations(%d) equals the number of relations checked",
>                MaxFSMRelations),
>                errhint("You have >= %d relations.\n"
>                         "Consider increasing the configuration parameter \"max_fsm_relations\".",
>                        numRels)));
>       else if (needed > MaxFSMPages)
> !         ereport(NOTICE,
>               (errmsg("the number of page slots needed (%.0f) exceeds max_fsm_pages (%d)",
>                needed,MaxFSMPages),
>                errhint("Consider increasing the configuration parameter \"max_fsm_relations\"\n"
> --- 713,736 ----
>                errdetail("FSM size: %d relations + %d pages = %.0f kB shared memory.",
>                          MaxFSMRelations, MaxFSMPages,
>                          (double) FreeSpaceShmemSize() / 1024.0)));
> !
> !     CheckFreeSpaceMapStatistics(NOTICE, numRels, needed);
> !     /* Print to server logs too because is deals with a config variable. */
> !     CheckFreeSpaceMapStatistics(LOG, numRels, needed);
> ! }
> !
> ! static void
> ! CheckFreeSpaceMapStatistics(int elevel, int numRels, double needed)
> ! {
> !        if (numRels == MaxFSMRelations)
> !         ereport(elevel,
>               (errmsg("max_fsm_relations(%d) equals the number of relations checked",
>                MaxFSMRelations),
>                errhint("You have >= %d relations.\n"
>                         "Consider increasing the configuration parameter \"max_fsm_relations\".",
>                        numRels)));
>       else if (needed > MaxFSMPages)
> !         ereport(elevel,
>               (errmsg("the number of page slots needed (%.0f) exceeds max_fsm_pages (%d)",
>                needed,MaxFSMPages),
>                errhint("Consider increasing the configuration parameter \"max_fsm_relations\"\n"

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I never heard any discussion on whether this should be backpatched to
> 8.0.X.  Should it?

I'm not inclined to throw it in at the last minute, as it's not been
through any testing and I'm not sure the behavior has really been agreed
on anyway.  (The diff you cite starts from code that's not in 8.0.* either.)

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
>>I never heard any discussion on whether this should be backpatched to
>>8.0.X.  Should it?

I personally think it should _not_ be backpatched.  Since it
doesn't fix any bugs, it's not really the kind of thing I
would expect to be backpatched.


> I'm not inclined to throw it in at the last minute, as it's not been
> through any testing and I'm not sure the behavior has really been agreed
> on anyway.  (The diff you cite starts from code that's not in 8.0.* either.)

Regarding the behavior I pretty much thought it was agreed upon.
I saw people proposing reasons advocating both the log file and
the client getting the message.   Simon's "Can we have both?"
comment got one positive response (Bruce's with the patch) and
no negative ones, I thought that indicated general agreement.

If we did want to re-open the behavior question, I might mention
that this message is only printed on a database-wide VACUUM; and
with autovacuum targeting specific tables such database-wide
VACUUMs might become more and more rare.  But I think that's a
separate issue.

     Ron

Ron Mayer wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >>I never heard any discussion on whether this should be backpatched to
> >>8.0.X.  Should it?
>
> I personally think it should _not_ be backpatched.  Since it
> doesn't fix any bugs, it's not really the kind of thing I
> would expect to be backpatched.
>

OK, just asking.

> > I'm not inclined to throw it in at the last minute, as it's not been
> > through any testing and I'm not sure the behavior has really been agreed
> > on anyway.  (The diff you cite starts from code that's not in 8.0.* either.)
>
> Regarding the behavior I pretty much thought it was agreed upon.
> I saw people proposing reasons advocating both the log file and
> the client getting the message.   Simon's "Can we have both?"
> comment got one positive response (Bruce's with the patch) and
> no negative ones, I thought that indicated general agreement.
>
> If we did want to re-open the behavior question, I might mention
> that this message is only printed on a database-wide VACUUM; and
> with autovacuum targeting specific tables such database-wide
> VACUUMs might become more and more rare.  But I think that's a
> separate issue.

Yea, I think we are agreed.  I was not asking for 8.0.2 but just 8.0.X
in general.  I ask only to see if someone jumps up and wants to explain
why it should  be in 8.0.X.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073