Re: [PATCHES] pg_autovacuum integration patch
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:[EMAIL PROTECTED] > 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 > > > > ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] pg_autovacuum integration patch
"Magnus Hagander" <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] pg_autovacuum integration patch
On Wed, 2004-06-16 at 10:01, Tom Lane wrote: > "Magnus Hagander" <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] pg_autovacuum integration patch
Tom Lane wrote: "Magnus Hagander" <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
