On Mon, 2009-11-30 at 14:38 +0900, Itagaki Takahiro wrote:
> > * "VACUUM (FULL REPLACE) pg_class" should be rejected, not silently
> > turned into "VACUUM (FULL INPLACE) pg_class".
>
> Hmmm, it requires to remember whether REPLACE is specified or not
> for the non-INPLACE vacuum, but I don't want to add VACOPT_REPLACE
> only for the purpose.
>
> I just removed "FULL REPLACE" syntax; Only "FULL" and "FULL INPLACE" are
> available. VACUUM FULL without INPLACE behaves as cluster-like rewrites
> for non-system tables. FULL INPLACE is a traditional vacuum full.
> System catalogs are always vacuumed with INPLACE version.
> - VACUUM FULL / VACUUM (FULL) => rewritten version
> - VACUUM (FULL INPLACE) => traditional version
Ok, looks good. It's cleaner now, too.
> It might make the code cleaner, but I want vacuumdb in 8.5 to work on older
> versions of servers unless we use the new feature. Older servers can only
> accept older syntax, so I avoided using the new syntax if not needed.
> (The new patch still uses two versions of syntax.)
Good point. I attached a suggestion of how it might look if you detected
the server version explicitly. You don't have to use it, but it's what I
had in mind.
Also, I think the current version fails if -i is passed and it's
connecting to an old server, so explicit server version detection may be
required.
> > * The patch should be merged with CVS HEAD. The changes required are
> > minor; but notice that there is a minor style difference in the assert
> > in vacuum().
Very minor style issue: it looks like Tom specifically changed the order
of the expression in the Assert() from your first vacuum options patch.
I attached a diff to show you what I mean -- the complex boolean
expressions are easier to read if the styles match.
> > * vacuumdb should reject -i without -f
> > * The replace or inplace option is a "magical" default, because "VACUUM
> > FULL" defaults to "replace" for user tables and "inplace" for system
> > tables. I tried to make that more clear in my documentation suggestions.
> > * There are some windows line endings in the patch, which should be
> > removed.
Great, thank you for the patch!
Marking as ready.
Regards,
Jeff Davis
