patch for pg_autovacuum 8.0.x prevent segv for dropped tables
Apologies if this is old news, but pg_autovacuum in 8.0.x has the bad habit
of SEGVing and exiting when a table gets dropped out from under it. This
creates problems if you rely on pg_autovacuum for the bulk of your vacuuming
as it forgets it's statistics when it is restarted and so will skip some
desireable vacuums.
I looked at the new autovacuum in 8.1 and it appears from casual inspection
not to have the same problem.
Below is a patch for this that should apply against any 8.0.x. The change
verifies that the catalog query returned some rows before accessing the row
data.
-dg
diff -Naur source/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c build/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c
--- source/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c 2005-04-02 16:02:03.000000000 -0800
+++ build/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c 2005-09-28 22:15:25.428710172 -0700
@@ -1013,6 +1013,7 @@
static void
perform_maintenance_command(db_info * dbi, tbl_info * tbl, int operation)
{
+ PGresult *res;
char buf[256]; /*
@@ -1069,10 +1070,16 @@
fflush(LOGOUTPUT);
}
- send_query(buf, dbi);
-
- update_table_thresholds(dbi, tbl, operation);
-
+ res = send_query(buf, dbi);
+ if (PQntuples(res)) {
+ update_table_thresholds(dbi, tbl, operation);
+ } else {
+ if (args->debug >= 1) {
+ sprintf(logbuffer, "Cannot refind table %s", tbl->table_name);
+ log_entry(logbuffer, LVL_DEBUG);
+ fflush(LOGOUTPUT);
+ }
+ }
if (args->debug >= 2)
print_table_info(tbl);
}
--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with insects.
Looks reasonable to me. All the patch does is make sure that the result
set is valid. Probably a check I should have done from the beginning,
or pg _autovacuum should be locking tables to make sure they aren't
dropped, but that sounds too intrusive, this is probably better.
Matt
daveg wrote:
Show quoted text
Apologies if this is old news, but pg_autovacuum in 8.0.x has the bad habit
of SEGVing and exiting when a table gets dropped out from under it. This
creates problems if you rely on pg_autovacuum for the bulk of your vacuuming
as it forgets it's statistics when it is restarted and so will skip some
desireable vacuums.I looked at the new autovacuum in 8.1 and it appears from casual inspection
not to have the same problem.Below is a patch for this that should apply against any 8.0.x. The change
verifies that the catalog query returned some rows before accessing the row
data.-dg
diff -Naur source/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c build/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c --- source/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c 2005-04-02 16:02:03.000000000 -0800 +++ build/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c 2005-09-28 22:15:25.428710172 -0700 @@ -1013,6 +1013,7 @@ static void perform_maintenance_command(db_info * dbi, tbl_info * tbl, int operation) { + PGresult *res; char buf[256];/*
@@ -1069,10 +1070,16 @@
fflush(LOGOUTPUT);
}- send_query(buf, dbi); - - update_table_thresholds(dbi, tbl, operation); - + res = send_query(buf, dbi); + if (PQntuples(res)) { + update_table_thresholds(dbi, tbl, operation); + } else { + if (args->debug >= 1) { + sprintf(logbuffer, "Cannot refind table %s", tbl->table_name); + log_entry(logbuffer, LVL_DEBUG); + fflush(LOGOUTPUT); + } + } if (args->debug >= 2) print_table_info(tbl); }
Small nit: please observe postgres community conventions regarding
a) indentation (BSD style, tabsize 4) and
b) diff type (context, not unidiff)
The patch itself looks ok to me.
cheers
andrew
daveg wrote:
Show quoted text
Apologies if this is old news, but pg_autovacuum in 8.0.x has the bad habit
of SEGVing and exiting when a table gets dropped out from under it. This
creates problems if you rely on pg_autovacuum for the bulk of your vacuuming
as it forgets it's statistics when it is restarted and so will skip some
desireable vacuums.I looked at the new autovacuum in 8.1 and it appears from casual inspection
not to have the same problem.Below is a patch for this that should apply against any 8.0.x. The change
verifies that the catalog query returned some rows before accessing the row
data.-dg
diff -Naur source/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c build/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c --- source/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c 2005-04-02 16:02:03.000000000 -0800 +++ build/postgresql-8.0.2/contrib/pg_autovacuum/pg_autovacuum.c 2005-09-28 22:15:25.428710172 -0700 @@ -1013,6 +1013,7 @@ static void perform_maintenance_command(db_info * dbi, tbl_info * tbl, int operation) { + PGresult *res; char buf[256];/*
@@ -1069,10 +1070,16 @@
fflush(LOGOUTPUT);
}- send_query(buf, dbi); - - update_table_thresholds(dbi, tbl, operation); - + res = send_query(buf, dbi); + if (PQntuples(res)) { + update_table_thresholds(dbi, tbl, operation); + } else { + if (args->debug >= 1) { + sprintf(logbuffer, "Cannot refind table %s", tbl->table_name); + log_entry(logbuffer, LVL_DEBUG); + fflush(LOGOUTPUT); + } + } if (args->debug >= 2) print_table_info(tbl); }
daveg <daveg@sonic.net> writes:
Below is a patch for this that should apply against any 8.0.x. The change
verifies that the catalog query returned some rows before accessing the row
data.
Surely this is completely broken? AFAICT you are testing the result
from a VACUUM or ANALYZE command, which is not going to return any
tuples.
regards, tom lane
Tom Lane wrote:
daveg <daveg@sonic.net> writes:
Below is a patch for this that should apply against any 8.0.x. The change
verifies that the catalog query returned some rows before accessing the row
data.Surely this is completely broken? AFAICT you are testing the result
from a VACUUM or ANALYZE command, which is not going to return any
tuples.
Upon further inspection, I think you are right. I would think that
instead of checking the query result with PQntuples, it should probably
be checked with |PQresultStatus.
Matt
|
"Matthew T. O'Connor" <matthew@zeut.net> writes:
Tom Lane wrote:
Surely this is completely broken? AFAICT you are testing the result
from a VACUUM or ANALYZE command, which is not going to return any
tuples.
Upon further inspection, I think you are right. I would think that
instead of checking the query result with PQntuples, it should probably
be checked with |PQresultStatus.
ISTM this is the wrong place to test at all. I put a PQntuples check
into update_table_thresholds() instead, which seems a far more direct
defense against trouble. (Consider eg the case where someone drops the
table just after your VACUUM completes successfully. Also there are
drop/rename scenarios to think about: success of the VACUUM proves that
there is a table named FOO, not that there is still a table with the OID
you have on record.)
regards, tom lane
Tom Lane wrote:
daveg <daveg@sonic.net> writes:
Below is a patch for this that should apply against any 8.0.x. The change
verifies that the catalog query returned some rows before accessing the row
data.Surely this is completely broken? AFAICT you are testing the result
from a VACUUM or ANALYZE command, which is not going to return any
tuples.
I guess he should change
if (PQntuples(res))
to
if (|PQresultStatus|(res) == PGRES_COMMAND_OK)
?
cheers
andrew
On Thu, Oct 20, 2005 at 12:30:27PM -0400, Tom Lane wrote:
"Matthew T. O'Connor" <matthew@zeut.net> writes:
Tom Lane wrote:
Surely this is completely broken? AFAICT you are testing the result
from a VACUUM or ANALYZE command, which is not going to return any
tuples.Upon further inspection, I think you are right. I would think that
instead of checking the query result with PQntuples, it should probably
be checked with |PQresultStatus.ISTM this is the wrong place to test at all. I put a PQntuples check
into update_table_thresholds() instead, which seems a far more direct
defense against trouble. (Consider eg the case where someone drops the
table just after your VACUUM completes successfully. Also there are
drop/rename scenarios to think about: success of the VACUUM proves that
there is a table named FOO, not that there is still a table with the OID
you have on record.)
Yes, I agree, update_table_thresholds() is the right place for the check.
Please ignore the earlier patch.
Thanks
-dg
--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with insects.