On Thu, Jun 17, 2021 at 10:54 AM Peter Geoghegan <p...@bowt.ie> wrote:
>
> On Sun, May 30, 2021 at 6:30 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> > We need to accept "yes" and "no" too? Currently, the parsing of a
> > boolean type reloption accepts those words.
>
> Added those in the attached revision, version 2. This is much closer
> to being commitable than v1 was. I plan on committing this in the next
> several days.
>
> I probably need to polish the documentation some more, though.
>
> > It seems to me that it's better to have INDEX_CLEANUP option of VACUUM
> > command support AUTO for consistency. Do you have any concerns about
> > supporting it?
>
> v2 sorts out the mess with VacOptTernaryValue by just adding a new
> enum constant to VacOptTernaryValue, called VACOPT_TERNARY_AUTO -- the
> enum still has a distinct VACOPT_TERNARY_DEFAULT value. v2 also adds a
> new reloption-specific enum, StdRdOptIndexCleanup, which is the
> datatype that we actually use inside the StdRdOptions struct. So we
> are now able to specify "VACUUM (INDEX_CLEANUP AUTO)" in v2 of the
> patch.
>
> v2 also adds a new option to vacuumdb, --force-index-cleanup. This
> seemed to make sense because we already have a --no-index-cleanup
> option.
>
> > > And does StdRdOptions.vacuum_truncate now need
> > > to become a VacOptTernaryValue field too, for consistency with the new
> > > definition of StdRdOptions.vacuum_index_cleanup?
> >
> > We don't have the bypass optimization for heap truncation, unlike
> > index vacuuming. So I think we can leave both vacuum_truncate
> > reloption and TRUNCATE option as boolean parameters.
>
> Actually FWIW we do have a bypass optimization for TRUNCATE -- it too
> has an always-on dynamic behavior -- so it really is like the index
> vacuuming thing. In theory it might make sense to have the same "auto,
> on, off" thing, just like with index vacuuming in the patch. However,
> I haven't done that in the patch because in practice it's a bad idea.
> If we offered users the option of truly forcing truncation, then
> lazy_truncate_heap() could just insist on truncating. It would have to
> just wait for an AEL, no matter how long it took. That would probably
> be dangerous because waiting for an AEL without backing out in VACUUM
> just isn't a great idea.
I agree that it doesn't make sense to force heap truncation.
Thank you for updating the patch! Here are comments on v2 patch:
typedef enum VacOptTernaryValue
{
VACOPT_TERNARY_DEFAULT = 0,
+ VACOPT_TERNARY_AUTO,
VACOPT_TERNARY_DISABLED,
VACOPT_TERNARY_ENABLED,
} VacOptTernaryValue;
VacOptTernaryValue is no longer a ternary value. Can we rename it
something like VacOptValue?
---
+ if (vacopts->force_index_cleanup)
{
- /* INDEX_CLEANUP is supported since v12 */
+ /*
+ * "INDEX_CLEANUP TRUE" has been supported since v12. Though
+ * the --force-index-cleanup vacuumdb option was only added in
+ * v14, it still works in the same way on v12+.
+ */
Assert(serverVersion >= 120000);
+ Assert(!vacopts->no_index_cleanup);
appendPQExpBuffer(sql, "%sINDEX_CLEANUP FALSE", sep);
sep = comma;
}
We should specify TRUE instead.
---
--force-index-cleanup option isn't shown in the help message.
---
I think we also improve the tab completion for INDEX_CLEANUP option.
---
@@ -32,7 +32,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [
<replaceable class="paramet
ANALYZE [ <replaceable class="parameter">boolean</replaceable> ]
DISABLE_PAGE_SKIPPING [ <replaceable
class="parameter">boolean</replaceable> ]
SKIP_LOCKED [ <replaceable class="parameter">boolean</replaceable> ]
- INDEX_CLEANUP [ <replaceable class="parameter">boolean</replaceable> ]
+ INDEX_CLEANUP [ <replaceable class="parameter">enum</replaceable> ]
PROCESS_TOAST [ <replaceable class="parameter">boolean</replaceable> ]
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
How about listing the available values of INDEX_CLEANUP here instead
of enum? For example, we do a similar thing in the description of
FORMAT option of EXPLAIN command. It would be easier to perceive all
available values.
---
+ <varlistentry>
+ <term><option>--no-index-cleanup</option></term>
+ <listitem>
+ <para>
It should be --force-index-cleanup.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
