Re: pg_autovacuum integration patch

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,