Thread: patch for pg_autovacuum 8.0.x prevent segv for dropped tables

patch for pg_autovacuum 8.0.x prevent segv for dropped tables

From

daveg

Date:

20 October 2005, 09:58:05

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.

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped

From

Matthew

Date:

20 October 2005, 14:33:47

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

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped

From

Andrew Dunstan

Date:

20 October 2005, 14:52:57

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

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped tables

From

Tom Lane

Date:

20 October 2005, 16:06:59

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

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped

From

"Matthew T. O'Connor"

Date:

20 October 2005, 16:26:58

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

|

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped tables

From

Tom Lane

Date:

20 October 2005, 16:30:31

"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

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped

From

Andrew Dunstan

Date:

20 October 2005, 16:34:10


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

Re: patch for pg_autovacuum 8.0.x prevent segv for dropped tables

From

daveg

Date:

20 October 2005, 19:38:47

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.