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
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
Show quoted text
-----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 patchOk, 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 thresholdsMatthew O'Connor
Import Notes
Resolved by subject fallback
"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,
Attachments:
difftext/x-patch; charset=utf-8; name=diffDownload+973-945
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