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.

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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.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);
> }
>


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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.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);
> }
>
>

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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

---------------------------(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

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

|

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

"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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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



---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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.

---------------------------(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