From: | Jeff Davis <pgsql(at)j-davis(dot)com> |
---|---|
To: | Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
Cc: | Alvaro Herrera <alvherre(at)commandprompt(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: New VACUUM FULL |
Date: | 2009-12-01 09:43:30 |
Message-ID: | 1259660610.3355.123.camel@jdavis |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
vacuum-assert-suggestion.context.patch | text/x-patch | 886 bytes |
vacuum-cli-suggestion.context.patch | text/x-patch | 2.4 KB |
vacuum-doc-suggestion.context.patch | text/x-patch | 2.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Marko Kreen | 2009-12-01 10:46:15 | Re: Application name patch - v4 |
Previous Message | Tatsuo Ishii | 2009-12-01 09:30:03 | Re: Application name patch - v4 |