Re: pg_autovacuum integration patch

From: "Magnus Hagander" <mha(at)sollentuna(dot)net>
To: "Matthew T(dot) O'Connor" <matthew(at)zeut(dot)net>, <pgsql-patches(at)postgresql(dot)org>
Subject: Re: pg_autovacuum integration patch
Date: 2004-06-16 09:14:00
Message-ID: 6BCB9D8A16AC4241919521715F4D8BCE1716B3@algol.sollentuna.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

Some nitpicking on details:

The comment above AutoVacMain() claims:
! * Main entry point for bgwriter process

I also see a bunch of // comments, I think those are not appreciated.

Haven't had time to look much at the actual functionality. Just did a
quick look-through for win32 showstoppers, to be honest - didn't find
any. (Doesn't mean they're not there, but..)

Also, isn't it a bit unnecessary to do:
sprintf(logbuffer,"foo bar %s",whatever);
elog(ERROR,logbuffer);

why not just
elog(ERROR,"foo bar %s",whatever);
[assuming I read the patch right - I still need some practice reading
patches...]

I also noticed:
! elog(ERROR, "pg_autovacuum: GUC variable stats_row_level
must be enabled.");
! elog(ERROR, " Please fix the problems and try
again.");

If you use the ereport() call instead of elog, you can set the second
one as a HINT. Since it's really the same error, I don't think you want
multiple errors logged...

I'll leave it to others to comment on the actual code - I'll take the
easy way out and do nitpicking instead :-) I'll try to test this on
win32 as soon as I have my tree back in compiling order.

//Magnus

> -----Original Message-----
> From: Matthew T. O'Connor [mailto:matthew(at)zeut(dot)net]
> Sent: Wednesday, June 16, 2004 8:19 AM
> To: PostgreSQL Patches
> Subject: [PATCHES] pg_autovacuum integration patch
>
> Ok, I have an 1st cut patch for pg_autovacuum integration
> into the backend. Please apply this patch to CVS or at least
> review and let me know what I need to change to get it
> applied to CVS.
>
> This patch requires that pg_autovacuum.c and .h are moved
> from contrib to src/backend/postmaster and
> src/include/postmaster respectively. I have also attached
> the pg_autovacuum.h file that needs to be put in
> src/include/catelog/ for the new pg_autovacuum system table.
>
> There is more to do for pg_autovacuum but I would like to get
> this patch included into CVS or at least get it rejected now
> so I can fix it before July 1.
>
> With this patch pg_autovacuum:
> * is a postmaster subprocess modeled after the bgwriter
> * uses elog for all output (I guessed at the appropriate elog levels)
> * used a new GUC variable to enable and disable pg_autovacuum
> * stores all it's volitile data in a new pg_autovacuum system table
> * allows admin to set per table thresholds
>
> Matthew O'Connor
>
>
>
>

Responses

Browse pgsql-patches by date

  From Date Subject
Next Message Claudio Natoli 2004-06-16 13:48:51 pg_ctl service integration for WIN32
Previous Message Laurent Ballester 2004-06-16 07:25:44 Re: eventlog fix