Re: pg_avd

On Tue, 2003-02-11 at 21:48, Matthew T. O'Connor wrote:
> Here is the code for the auto vacuum daemon I'm working on.

Some minor nit-picking follows below. I tend to be a bit of a
style-nazi, don't mind me :-)

- the length argument to snprintf() includes the terminating NUL byte,
so code like pg_avd.c line 334 is off-by-one:

  char buf[256];
  /* ... */
  snprintf(buf,255,"...");

- AFAICS, there is no reason for update_db_list() and append_new_dbs()
to use more than 1 database connection and 1 database query between them
(just fetch the list of all DBs and the appropriate stats, then loop
through the in-memory list of DB information, removing no-longer-present
DBs and adding newly-created DBs).

- if you're going to use the 'constant == variable' technique for
comparisons, it would be nice to use it consistently

- the usage info for "-h" is incorrect, as is the name of the app
mentioned in the usage info ("pg_avd", not "pg_avd_c")

- the return value of init_tables() is not what the comment states it
should be

- if all the pg_avd functions are only used by pg_avd itself, why not
make them static?

More significant changes I think would be useful:

- separating the logic for ANALYZE and VACUUM seems like a good idea,
IMHO. For example, INSERT doesn't create any dead tuples, so it
shouldn't effect the need to VACUUM in any way -- but enough INSERTs
will eventually warrant an ANALYZE. Also, I'd think most installations
will need to VACUUM more often than they'll need to ANALYZE, so doing
both at the same time doesn't seem optimal.

- using some of the ideas from contrib/pgstattuple (i.e. looking at the
amount of "dead space" in the relation) could be an interesting
enhancement.

As far as where this belongs, I vote against it going into bin/. It
isn't polished enough, either in concept or in implementation, to
deserve that kind of endorsement. But I think putting it into contrib/
for the next release would be a good idea: if people like it, we can
take a look at seeing what other features / fine-tuning it needs to
warrant being part of the official package.

Cheers,

Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC