Skip site navigation (1) Skip section navigation (2)

Re: New VACUUM FULL

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 (view raw or flat)
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: vacuum-doc-suggestion.context.patch
Description: text/x-patch (2.3 KB)
Attachment: vacuum-cli-suggestion.context.patch
Description: text/x-patch (2.4 KB)
Attachment: vacuum-assert-suggestion.context.patch
Description: text/x-patch (886 bytes)

In response to

Responses

pgsql-hackers by date

Next:From: Marko KreenDate: 2009-12-01 10:46:15
Subject: Re: Application name patch - v4
Previous:From: Tatsuo IshiiDate: 2009-12-01 09:30:03
Subject: Re: Application name patch - v4

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group