Re: New VACUUM FULL

Thanks for review!

Jeff Davis <pgsql@j-davis.com> 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

>  * In vacuumdb, the code for handling INPLACE is a little ugly. Perhaps
> it should be changed to always use your new options syntax? That might
> be more code, but it would be a little more readable.

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

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

Done.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center