Re: Teaching users how they can get the most out of HOT in Postgres 14 - Mailing list pgsql-hackers

On Fri, May 28, 2021 at 9:53 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Sun, May 23, 2021 at 11:34 PM Masahiko Sawada <sawada.mshk@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/