Thread: pg_autovacuum integration patch

pg_autovacuum integration patch

From
"Matthew T. O'Connor"
Date:
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



Attachment

Re: pg_autovacuum integration patch

From
"Magnus Hagander"
Date:
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@zeut.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
>
>
>
>

Re: pg_autovacuum integration patch

From
Tom Lane
Date:
"Magnus Hagander" <mha@sollentuna.net> writes:
> 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...

Even more to the point, control will never *reach* the second elog().
Matthew clearly needs to spend more time studying the backend error
message reporting mechanism.  There is some documentation here:
http://developer.postgresql.org/docs/postgres/error-message-reporting.html
and the backend code is certainly chock-full of examples.

            regards, tom lane

Re: pg_autovacuum integration patch

From
"Matthew T. O'Connor"
Date:
Ok I have fixed most of what Mangnus mentioned and attached an updated
diff file.


On Wed, 2004-06-16 at 05:14, Magnus Hagander wrote:
> Some nitpicking on details:
>
> The comment above AutoVacMain() claims:
> !  * Main entry point for bgwriter process

Oops... Can you tell where I cribbed the code from?  Fixed in attached
patch.

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

Yeah ok fixed in attached patch.

> 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...]

Sorry this was due to my ignorance of the elog / ereport api, I'll fixed
in attached patch.

> 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...

Ok, I didn't have time to change hings to ereport, I'll look into this tonight.

> 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.

Thanks for the feedback. Please do test this on win32.

Thanks again,



Attachment

Re: pg_autovacuum integration patch

From
"Matthew T. O'Connor"
Date:
On Wed, 2004-06-16 at 10:01, Tom Lane wrote:
> "Magnus Hagander" <mha@sollentuna.net> writes:
> > 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...
>
> Even more to the point, control will never *reach* the second elog().
> Matthew clearly needs to spend more time studying the backend error
> message reporting mechanism.  There is some documentation here:
> http://developer.postgresql.org/docs/postgres/error-message-reporting.html
> and the backend code is certainly chock-full of examples.

No doubt, I'll try to take a look at this soon.  Since the deadline is
coming up, I wanted to find out my shortcoming sooner rather than
later.

Put another way, I thought it was time to stop operating in a "VACUUM"
;-)

Matthew



Re: pg_autovacuum integration patch

From
Andrew Dunstan
Date:
Tom Lane wrote:

>"Magnus Hagander" <mha@sollentuna.net> writes:
>
>
>>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...
>>
>>
>
>Even more to the point, control will never *reach* the second elog().
>Matthew clearly needs to spend more time studying the backend error
>message reporting mechanism.  There is some documentation here:
>http://developer.postgresql.org/docs/postgres/error-message-reporting.html
>and the backend code is certainly chock-full of examples.
>
>
>

Perhaps it's just as well that it isn't reached :-) "Please fix the
problems and try again" doesn't strike me as a very useful message.

cheers

andrew