Review comments:

 * I attached some documentation suggestions.
 * 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.
 * "VACUUM (FULL REPLACE) pg_class" should be rejected, not silently
turned into "VACUUM (FULL INPLACE) pg_class".
 * There are some windows line endings in the patch, which should be
removed.
 * 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.

Regards,
        Jeff Davis

*** a/doc/src/sgml/ref/vacuum.sgml
--- b/doc/src/sgml/ref/vacuum.sgml
***************
*** 22,29 **** PostgreSQL documentation
   <refsynopsisdiv>
  <synopsis>
  VACUUM [ ( { FULL [ INPLACE | REPLACE ] | FREEZE | VERBOSE | ANALYZE } [, ...] ) ] [ <replaceable class="PARAMETER">table</replaceable> [ (<replaceable class="PARAMETER">column</replaceable> [, ...] ) ] ]
! VACUUM [ FULL [ INPLACE | REPLACE ] ] [ FREEZE ] [ VERBOSE ] [ <replaceable class="PARAMETER">table</replaceable> ]
! VACUUM [ FULL [ INPLACE | REPLACE ] ] [ FREEZE ] [ VERBOSE ] ANALYZE [ <replaceable class="PARAMETER">table</replaceable> [ (<replaceable class="PARAMETER">column</replaceable> [, ...] ) ] ]
  </synopsis>
   </refsynopsisdiv>
  
--- 22,29 ----
   <refsynopsisdiv>
  <synopsis>
  VACUUM [ ( { FULL [ INPLACE | REPLACE ] | FREEZE | VERBOSE | ANALYZE } [, ...] ) ] [ <replaceable class="PARAMETER">table</replaceable> [ (<replaceable class="PARAMETER">column</replaceable> [, ...] ) ] ]
! VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ <replaceable class="PARAMETER">table</replaceable> ]
! VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ <replaceable class="PARAMETER">table</replaceable> [ (<replaceable class="PARAMETER">column</replaceable> [, ...] ) ] ]
  </synopsis>
   </refsynopsisdiv>
  
***************
*** 85,101 **** VACUUM [ FULL [ INPLACE | REPLACE ] ] [ FREEZE ] [ VERBOSE ] ANALYZE [ <replacea
       <para>
        Selects <quote>full</quote> vacuum, which can reclaim more
        space, but takes much longer and exclusively locks the table.
-       <literal>INPLACE</literal> option consumes less disk space but takes
-       longer time, and <literal>REPLACE</literal> option vice versa.
-       The default is <literal>REPLACE</literal>.
       </para>
       <para>
!       <command>VACUUM (FULL INPLACE)</command> behaves as a traditional
!       <command>VACUUM FULL</command> command. It moves tuples in the table
!       from the tail to the head of files. On the other hand,
!       <command>VACUUM (FULL REPLACE)</command> behaves as a
!       <command>CLUSTER</command> command without any sorting.
!       It moves all tuples into a new table and swap the old and the new tables.
       </para>
      </listitem>
     </varlistentry>
--- 85,107 ----
       <para>
        Selects <quote>full</quote> vacuum, which can reclaim more
        space, but takes much longer and exclusively locks the table.
       </para>
       <para>
!       If <literal>REPLACE</literal> is specified, the entire table and
!       all indexes are rewritten. This method requires extra disk space
!       in which to write the new data, and is generally most efficient
!       when a significant amount of space needs to be reclaimed from
!       within the table. This method is the default for user tables,
!       but it cannot be used on system catalogs.
!      </para>
!      <para>
!       If <literal>INPLACE</literal> is specified, the table and
!       indexes are modified in place to reclaim space. This method may
!       require less disk space than <literal>FULL REPLACE</literal> for
!       the table data, but the indexes will grow which may counteract
!       that benefit. <literal>FULL INPLACE</literal> is generally
!       slower, and should only be used for system catalogs (for which
!       it is the default).
       </para>
      </listitem>
     </varlistentry>
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to