Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped tables
On Thu, Oct 20, 2005 at 12:30:27PM -0400, Tom Lane wrote: > "Matthew T. O'Connor" 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 [EMAIL PROTECTED] If simplicity worked, the world would be overrun with insects. ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped
Tom Lane wrote: daveg <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped tables
"Matthew T. O'Connor" 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped
Tom Lane wrote: daveg <[EMAIL PROTECTED]> 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 | ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped tables
daveg <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped
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:
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.0 -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;
charbuf[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);
}
---(end of broadcast)---
TIP 6: explain analyze is your friend
Re: [PATCHES] patch for pg_autovacuum 8.0.x prevent segv for dropped
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:
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.0 -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;
charbuf[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);
}
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
