Re: New VACUUM FULL

On Tue, 2009-10-27 at 13:55 +0900, Itagaki Takahiro wrote:
> Here is a patch to support "rewrite" version of VACUUM FULL.

Can you please provide a patch that applies cleanly on top of the vacuum
options patch and on top of CVS HEAD (there are a couple minor conflicts
with this version). That would make review easier.

Initial comments:

1. Do we want to introduce syntax for INPLACE at all, if we are
eventually going to remove the current mechanism? If not, then we should
probably use REWRITE, because that's a better word than "REPLACE", I
think.

My opinion is that if we really still need the current in-place
mechanism, then VACUUM (FULL) should use the current in-place mechanism;
and VACUUM (FULL REWRITE) should use your new rewrite mechanism. That
gives us a nice migration path toward always using the rewrite
mechanism.

2. Why do all of the following exist: VACOPT_FULL, VACOPT_REPLACE, and
VACOPT_INPLACE? Shouldn't VACOPT_FULL be equivalent to one of the other
two? This is essentially what Simon was getting at, I think.

3. Some options are being set in vacuum() itself. It looks like the
options should already be set in gram.y, so should that be an Assert
instead? I think it's cleaner to set all of the options properly early
on, rather than waiting until vacuum() to interpret the combinations.

I haven't looked at the implementation in detail yet, but at a glance,
it seems to be a good approach.

Regards,
Jeff Davis