New VACUUM FULL
in [PgSql]
|
From: Jeff Davis on 14 Nov 2009 20:26 On Sat, 2009-11-14 at 19:25 -0500, Tom Lane wrote: > As a rule of thumb, I'd recommend keeping as much complexity as possible > *out* of gram.y. It's best if that code just reports the facts, ie what > options the user entered. Deriving conclusions from that (like what > default behaviors should be used) is best done later. One example of > why is that if you want the defaults to depend on GUC settings then > that logic must *not* happen in gram.y, since the parse tree might > outlive the current GUC settings. I was referring to (in vacuum()): + if (vacstmt->options & (VACOPT_INPLACE | VACOPT_REPLACE | VACOPT_FREEZE)) + vacstmt->options |= VACOPT_VACUUM; + if (vacstmt->va_cols) + vacstmt->options |= VACOPT_ANALYZE; I think what you say applies to VACOPT_ANALYZE, which is implied when columns are supplied by the user but ANALYZE is not specified explicitly. In that case it should be set in vacuum() but not in gram.y (unless specified by the user). However, for VACOPT_VACUUM, I think that's still in the territory of gram.y. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Itagaki Takahiro on 15 Nov 2009 23:37 Jeff Davis <pgsql(a)j-davis.com> wrote: > You left INPLACE in the patch Oops, removed. > Sounds fine, but worth a mention in the documentation. Just add to the > "columns" part of the VACUUM page something like: "If specified, implies > ANALYZE". Added. > Other than these two minor issues, I don't see any problems with the > patch. Please post an updated version to the new commitfest entry. All of the vacuum options are adjusted in gram.y in the current patch. We only check the options with assertions in vacuum(): /* sanity checks */ Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE)); Assert(!(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)) || (vacstmt->options & VACOPT_VACUUM)); Assert(vacstmt->va_cols == NIL || (vacstmt->options & VACOPT_ANALYZE)); Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
From: Jeff Davis on 16 Nov 2009 00:59 On Mon, 2009-11-16 at 13:37 +0900, Itagaki Takahiro wrote: > Jeff Davis <pgsql(a)j-davis.com> wrote: > > > You left INPLACE in the patch > > Oops, removed. > > > Sounds fine, but worth a mention in the documentation. Just add to the > > "columns" part of the VACUUM page something like: "If specified, implies > > ANALYZE". > > Added. > Great, I am marking this part "ready for committer". I may be slow to review the remainder of the VACUUM FULL rewrite patch due to the conference in Tokyo, but I will review it soon afterward. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Itagaki Takahiro on 16 Nov 2009 04:29 Here is an updated patch of rewriting vacuum based on vacuum options patch. Documentations and vacuumdb modification (-i, --inplace) are added. Jeff Davis <pgsql(a)j-davis.com> wrote: > 1. Do we want to introduce syntax for INPLACE at all, if we are > eventually going to remove the current mechanism? > 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. AFAIK, "VACUUM FULL" should behave as "REWRITE" in the past discussion. Since we don't want users to use in-place FULL vacuum, so we will change the default behavior of VACUUM FULL. There are some choices: <REWRITE version> <in-place version> 1. VACUUM (FULL REPLACE) vs. VACUUM (FULL INPLACE) 2. VACUUM (FULL) vs. VACUUM (FULL INPLACE) 3. VACUUM (REWRITE) vs. VACUUM (FULL) 4. VACUUM (FULL REWRITE) vs. VACUUM (FULL) 5. Don't use SQL and use a GUC instead. (bool inplace_vacuum_full ?) I choose a hybrid syntax of 1 + 2 in the patch, but I'm not particular about it. What is the best? > 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. * FULL [REPLACE] := VACOPT_FULL * FULL INPLACE := VACOPT_FULL + VACOPT_INPLACE > 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 moved all of the logic into gram.y. vacuum() has only assert tests. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
From: Tom Lane on 16 Nov 2009 16:33
Jeff Davis <pgsql(a)j-davis.com> writes: > On Mon, 2009-11-16 at 13:37 +0900, Itagaki Takahiro wrote: >> [ new options syntax for VACUUM ] > Great, I am marking this part "ready for committer". Applied with very minor editorialization. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |