Re: pg_autovacuum integration patch - Mailing list pgsql-patches

From Matthew T. O'Connor
Subject Re: pg_autovacuum integration patch
Date
Msg-id 1087394677.20076.25.camel@zedora2
Whole thread Raw
In response to Re: pg_autovacuum integration patch  ("Magnus Hagander" <mha@sollentuna.net>)
List pgsql-patches
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

pgsql-patches by date:

Previous
From: Tom Lane
Date:
Subject: Re: pg_autovacuum integration patch
Next
From: "Matthew T. O'Connor"
Date:
Subject: Re: pg_autovacuum integration patch