Thread: 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
Attachment
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 > > > >
"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
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
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
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