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
*** a/src/backend/commands/vacuum.c
--- b/src/backend/commands/vacuum.c
***************
*** 303,310 **** vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
Assert((vacstmt->options & VACOPT_VACUUM) ||
!(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL);
! Assert(!(vacstmt->options & VACOPT_INPLACE) ||
! (vacstmt->options & VACOPT_FULL));
stmttype = (vacstmt->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
--- 303,310 ----
Assert((vacstmt->options & VACOPT_VACUUM) ||
!(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL);
! Assert((vacstmt->options & VACOPT_FULL) ||
! !(vacstmt->options & VACOPT_INPLACE));
stmttype = (vacstmt->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
*** a/src/bin/scripts/vacuumdb.c
--- b/src/bin/scripts/vacuumdb.c
***************
*** 203,225 **** vacuum_one_database(const char *dbname, bool full, bool inplace, bool verbose,
PQExpBufferData sql;
PGconn *conn;
initPQExpBuffer(&sql);
appendPQExpBuffer(&sql, "VACUUM");
! if (inplace)
{
! appendPQExpBuffer(&sql, " (FULL INPLACE");
if (freeze)
! appendPQExpBuffer(&sql, ", FREEZE");
if (verbose)
! appendPQExpBuffer(&sql, ", VERBOSE");
if (analyze)
! appendPQExpBuffer(&sql, ", ANALYZE");
! appendPQExpBuffer(&sql, ")");
}
else
{
if (full)
appendPQExpBuffer(&sql, " FULL");
if (freeze)
--- 203,250 ----
PQExpBufferData sql;
PGconn *conn;
+ int version;
+ bool first_opt = true;
initPQExpBuffer(&sql);
+ conn = connectDatabase(dbname, host, port, username, prompt_password, progname);
+ version = PQserverVersion(conn);
+
appendPQExpBuffer(&sql, "VACUUM");
!
! if (version >= 80500)
{
! if (full)
! {
! appendPQExpBuffer(&sql, "%sFULL%s", first_opt ? " (" : ", ",
! inplace ? " INPLACE" : "");
! first_opt = false;
! }
if (freeze)
! {
! appendPQExpBuffer(&sql, "%sFREEZE", first_opt ? " (" : ", ");
! first_opt = false;
! }
if (verbose)
! {
! appendPQExpBuffer(&sql, "%sVERBOSE", first_opt ? " (" : ", ");
! first_opt = false;
! }
if (analyze)
! {
! appendPQExpBuffer(&sql, "%sANALYZE", first_opt ? " (" : ", ");
! first_opt = false;
! }
! if (!first_opt)
! appendPQExpBuffer(&sql, ")");
}
else
{
+ /*
+ * On older servers, VACUUM FULL is equivalent to VACUUM (FULL
+ * INPLACE) on newer servers, so we can ignore 'inplace'.
+ */
if (full)
appendPQExpBuffer(&sql, " FULL");
if (freeze)
***************
*** 229,239 **** vacuum_one_database(const char *dbname, bool full, bool inplace, bool verbose,
if (analyze)
appendPQExpBuffer(&sql, " ANALYZE");
}
if (table)
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBuffer(&sql, ";\n");
- conn = connectDatabase(dbname, host, port, username, prompt_password, progname);
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
if (table)
--- 254,264 ----
if (analyze)
appendPQExpBuffer(&sql, " ANALYZE");
}
+
if (table)
appendPQExpBuffer(&sql, " %s", table);
appendPQExpBuffer(&sql, ";\n");
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
if (table)
*** a/doc/src/sgml/ref/vacuum.sgml
--- b/doc/src/sgml/ref/vacuum.sgml
***************
*** 87,107 **** VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ <replaceable class="PARAMETER">
space, but takes much longer and exclusively locks the table.
</para>
<para>
! If <literal>INPLACE</literal> is not 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 non-<literal>INPLACE</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>
--- 87,111 ----
space, but takes much longer and exclusively locks the table.
</para>
<para>
! For user tables, all table data and indexes are rewritten. This
! method requires extra disk space in which to write the new data,
! and is generally useful when a significant amount of space needs
! to be reclaimed from within the table.
</para>
<para>
! For system tables, all table data and indexes are modified in
! place to reclaim space. This method may require less disk space
! for the table data than <command>VACUUM FULL</command> on a
! comparable user table, but the indexes will grow which may
! counteract that benefit. Additionally, the operation is often
! slower than <command>VACUUM FULL</command> on a comparable user
! table.
! </para>
! <para>
! If <literal>FULL INPLACE</literal> is specified, the space is
! reclaimed in the same manner as a system table, even if it is a
! user table. Specifying <literal>INPLACE</literal> explicitly is
! rarely useful.
</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
