On Fri, 2009-12-04 at 09:20 +0000, Simon Riggs wrote:
> On Tue, 2009-12-01 at 01:43 -0800, Jeff Davis wrote:
> > Marking as ready.
>
> You're saying its ready, yet there are 3 additional suggestions patches
> attached here. Please can you resolve these and re-submit a single final
> patch from author and reviewer?
My apologies. At the time, I thought a couple days might matter, and the
changes are in areas that committers tend to editorialize anyway: docs
and a style issue. The only substantial patch was to vacuumdb.c.
Complete patch attached including my edits.
> * What happens if you issue VACUUM FULL; which we would expect to use
> the new method of vacuum on all tables in the database. Won't that just
> fail with an error when it comes to catalog tables? Sounds to me like we
> should avoid the error and just silently do an INPLACE on catalog
> tables.
That's how it works.
> * Such a pivotal change to Postgres needs more test coverage than a
> single line in regression tests. It might have been OK before, but I
> think we need a few more combinations here, at least in this release:
> with, without indexes, empty table, clustered, non-clustered etc and of
> course a full database VACUUM so that we have the catalog table case
> covered, plus an explicit catalog table vacuum.
It was my impression that the regression tests aren't meant to be
exhaustive, but merely exercise a good portion of the code to help
detect simple breakage. Also, pg_regress isn't good for detecting a lot
of the problems that vacuum might have (how do you even know whether the
vacuum happened in-place or not?).
We could put a VACUUM FULL; and a VACUUM (FULL INPLACE); somewhere,
which will cover a lot of the cases you're talking about. However, that
may be a performance consideration especially for people who develop on
laptops.
In general, though, I think the right place for this is a longer test
suite that is meant to be run less frequently.
Regards,
Jeff Davis
