Re: pg_avd - Mailing list pgsql-patches

Thank you much for the feedback / code review.

On Tue, 2003-02-18 at 03:24, Neil Conway wrote:
> 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,"...");

Will Fix, (actuall I will fix it as Tom suggested in the subsequent
email)

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

Agreed, will do.

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

Ok, I'll try to find all the places where it's not consistent.  The next
version I submit should be better.

> - 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")

Opps....

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

OK.

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

Ok will do.

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

Agreed, I have some of the infrastructure for this inplace (it tracks
inserts and deletes separately (update counts as one of each), and I
have a different threshold for both inserts and deletes, I'll finish
this off for the next version.

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

I did a little looking at this a while ago, it wasn't clear to me that
pgstattuple was any faster than just doing a vacuum.

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

Well I agree the implementation is not polished enough, but I can keep
working on that till everyone is happy.  The question I really have is
the concept.  However I feel I'm getting enough positive feedback to
keep working on it for inclusion into contrib, possibly bin but I agree
that one cycle in contrib might be a good idea.