Re: pg_autovacuum patch for 7.4.2 and HEAD

As reported by Cott Lang on 1/17
( http://archives.postgresql.org/pgsql-bugs/2004-01/msg00111.php )
pg_autovacuum.c has some problems with int overflow and not using appropriate
datatypes to track certain table values. This patch attempts to fix all
these issues. Someone should take a look and make sure its valid.

Matthew T. O'Connor

"Matthew T. O'Connor" <matthew(at)zeut(dot)net> writes:
> pg_autovacuum.c has some problems with int overflow and not using appropriate
> datatypes to track certain table values. This patch attempts to fix all
> these issues. Someone should take a look and make sure its valid.

> ! new_tbl->relid = atol(PQgetvalue(res, row, PQfnumber(res, "oid")));
> ! new_tbl->reltuples = atof(PQgetvalue(res, row, PQfnumber(res, "reltuples")));
> ! new_tbl->relpages = atol(PQgetvalue(res, row, PQfnumber(res, "relpages")));

This ignores the fact that relid and relpages are unsigned. I would
suggest adopting the same convention for OID as is used in pg_dump and
other places:

#define atooid(x) ((Oid) strtoul((x), NULL, 10))

You could actually use this same macro for reading relpages, but that's
probably abusing the notation. I'd use strtoul directly for relpages,
I think.

> ! init_dbinfo(char *dbname, int oid, int age)
> ...
> ! init_dbinfo(char *dbname, uint oid, uint age)

This (and other declarations) should be "Oid oid".

regards, tom lane

Tom Lane wrote:

>"Matthew T. O'Connor" <matthew(at)zeut(dot)net> writes:
>
>
>>! new_tbl->relid = atol(PQgetvalue(res, row, PQfnumber(res, "oid")));
>>! new_tbl->reltuples = atof(PQgetvalue(res, row, PQfnumber(res, "reltuples")));
>>! new_tbl->relpages = atol(PQgetvalue(res, row, PQfnumber(res, "relpages")));
>>
>>
>
>This ignores the fact that relid and relpages are unsigned. I would
>suggest adopting the same convention for OID as is used in pg_dump and
>other places:
>
>#define atooid(x) ((Oid) strtoul((x), NULL, 10))
>
>You could actually use this same macro for reading relpages, but that's
>probably abusing the notation. I'd use strtoul directly for relpages,
>I think.
>
>
>
>>! init_dbinfo(char *dbname, int oid, int age)
>>...
>>! init_dbinfo(char *dbname, uint oid, uint age)
>>
>>
>
>This (and other declarations) should be "Oid oid".
>
>
>

Thanks for the help / review. Here is my 2nd cut at fixing this. I
believe I have addressed the above concernes. Please review again and
(hopefully) apply.

Matthew

Tom Lane wrote:

>> "Matthew T. O'Connor" <matthew(at)zeut(dot)net> writes:
>>
>>> ! new_tbl->relid = atol(PQgetvalue(res, row, PQfnumber(res,
>>> "oid")));
>>> ! new_tbl->reltuples = atof(PQgetvalue(res, row, PQfnumber(res,
>>> "reltuples")));
>>> ! new_tbl->relpages = atol(PQgetvalue(res, row, PQfnumber(res,
>>> "relpages")));
>>
>>
>> This ignores the fact that relid and relpages are unsigned. I would
>> suggest adopting the same convention for OID as is used in pg_dump and
>> other places:
>>
>> #define atooid(x) ((Oid) strtoul((x), NULL, 10))
>>
>> You could actually use this same macro for reading relpages, but that's
>> probably abusing the notation. I'd use strtoul directly for relpages,
>> I think.
>>
>>> ! init_dbinfo(char *dbname, int oid, int age)
>>> ...
>>> ! init_dbinfo(char *dbname, uint oid, uint age)
>>>
>>
>>
>> This (and other declarations) should be "Oid oid".
>
>
> Thanks for the help / review. Here is my 2nd cut at fixing this. I
> believe I have addressed the above concernes. Please review again and
> (hopefully) apply.

Ack... sorry. This time with the attachment....

Patch applied. Thanks.

---------------------------------------------------------------------------

Matthew T. O'Connor wrote:
> Tom Lane wrote:
>
> >"Matthew T. O'Connor" <matthew(at)zeut(dot)net> writes:
> >
> >
> >>! new_tbl->relid = atol(PQgetvalue(res, row, PQfnumber(res, "oid")));
> >>! new_tbl->reltuples = atof(PQgetvalue(res, row, PQfnumber(res, "reltuples")));
> >>! new_tbl->relpages = atol(PQgetvalue(res, row, PQfnumber(res, "relpages")));
> >>
> >>
> >
> >This ignores the fact that relid and relpages are unsigned. I would
> >suggest adopting the same convention for OID as is used in pg_dump and
> >other places:
> >
> >#define atooid(x) ((Oid) strtoul((x), NULL, 10))
> >
> >You could actually use this same macro for reading relpages, but that's
> >probably abusing the notation. I'd use strtoul directly for relpages,
> >I think.
> >
> >
> >
> >>! init_dbinfo(char *dbname, int oid, int age)
> >>...
> >>! init_dbinfo(char *dbname, uint oid, uint age)
> >>
> >>
> >
> >This (and other declarations) should be "Oid oid".
> >
> >
> >
>
> Thanks for the help / review. Here is my 2nd cut at fixing this. I
> believe I have addressed the above concernes. Please review again and
> (hopefully) apply.
>
>
> Matthew
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073