On Fri, May 28, 2021 at 9:53 AM Peter Geoghegan <p...@bowt.ie> wrote:
>
> On Sun, May 23, 2021 at 11:34 PM Masahiko Sawada <sawada.m...@gmail.com>
> wrote:
> > I think the possible side effect of this hard-coded
> > BYPASS_THRESHOLD_PAGES would be that by default, bulkdelete is not
> > called for a long term and the index becomes bloat.
>
> What do you think of the approach taken in the attached POC patch?
>
> The patch makes it possible to disable the optimization by
> generalizing the INDEX_CLEANUP reloption to be an enum that looks like
> a trinay boolean (not just a plain boolean). INDEX_CLEANUP now accepts
> the values 'auto', 'on', and 'off' (plus a variety of alternative
> spellings, the usual ones for booleans in Postgres). Now 'auto' is the
> default, and 'on' forces the previous behavior inside vacuumlazy.c. It
> does not disable the failsafe, though -- INDEX_CLEANUP remains a
> fairly mechanical thing.
+1
>
> This approach seems good to me because INDEX_CLEANUP remains
> consistent with the original purpose and design of INDEX_CLEANUP --
> that was always an option that forced VACUUM to do something special
> with indexes. I don't see much downside to this approach, either. As
> things stand, INDEX_CLEANUP is mostly superseded by the failsafe, so
> we don't really need to talk about wraparound emergencies in the docs
> for INDEX_CLEANUP anymore. This seems much more elegant than either
> repurposing/reviving cleanup_index_scale_factor (which makes no sense
> to me at all) or inventing a new reloption (which would itself be in
> tension with INDEX_CLEANUP).
+1
>
> There are some practical issues that make this patch surprisingly
> complicated for such a simple problem. For example, I hope that I
> haven't missed any subtlety in generalizing a boolean reloption like
> this. We've done similar things with GUCs in the past, but this may be
> a little different.
+/* values from HeapOptIndexCleanupMode */
+relopt_enum_elt_def HeapOptIndexCleanupOptValues[] =
+{
+ {"auto", VACOPT_TERNARY_DEFAULT},
+ {"on", VACOPT_TERNARY_ENABLED},
+ {"off", VACOPT_TERNARY_DISABLED},
+ {"true", VACOPT_TERNARY_ENABLED},
+ {"false", VACOPT_TERNARY_DISABLED},
+ {"1", VACOPT_TERNARY_ENABLED},
+ {"0", VACOPT_TERNARY_DISABLED},
+ {(const char *) NULL} /* list terminator */
+};
We need to accept "yes" and "no" too? Currently, the parsing of a
boolean type reloption accepts those words.
> Another concern with this approach is what it
> means for the VACUUM command itself. I haven't added an 'auto'
> spelling that is accepted by the VACUUM command in this POC version.
> But do I need to at all? Can that just be implied by not having any
> INDEX_CLEANUP option?
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?
> 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.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
