Re: 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(at)sonic(dot)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:
> 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:

>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(at)sonic(dot)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(at)sonic(dot)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(at)zeut(dot)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(at)sonic(dot)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(at)zeut(dot)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(at)sonic(dot)net
If simplicity worked, the world would be overrun with insects.