Re: interval_ops shall stop using btequalimage (deduplication)

On Wed, Oct 11, 2023 at 11:38 AM Noah Misch <noah(at)leadboat(dot)com> wrote:
> Interesting. So, >99% of interval-type indexes, even ones WITH
> (deduplicate_items=off), will get amcheck failures. The <1% of exceptions
> might include indexes having allequalimage=off due to an additional column,
> e.g. a two-column (interval, numeric) index. If interval indexes are common
> enough and "pg_amcheck --heapallindexed" failures from $SUBJECT are relatively
> rare, that could argue for giving amcheck a special case. Specifically,
> downgrade its "metapage incorrectly indicates that deduplication is safe" from
> ERROR to WARNING for interval_ops only.

I am not aware of any user actually running "deduplicate_items = off"
in production, for any index. It was added purely as a defensive thing
-- not because I anticipated any real need to disable deduplication.
Deduplication was optimized for being enabled by default.

Anything is possible, of course, but it's hard to give too much weight
to cases where two very unlikely things happen to intersect. (Plus
"deduplicate_items = off" doesn't really do that much; more on that
below.)

> Without that special case (i.e. with
> the v1 patch), the release notes should probably resemble, "After updating,
> run REINDEX on all indexes having an interval-type column."

+1

> There's little
> point in recommending pg_amcheck if >99% will fail. I'm inclined to bet that
> interval-type indexes are rare, so I lean against adding the amcheck special
> case. It's not a strong preference. Other opinions?

Well, there will only be one known reason why anybody will ever see
this test fail (barring a near-miraculous coincidence where "generic
corruption" somehow gets passed our most basic sanity tests, only to
fail this metapage field check a bit later on). Even if you
pessimistically assume that similar problems remain undiscovered in
some other opfamily, this particular check isn't going to be the check
that detects the other problems -- you really would need
heapallindexed verification for that.

In short, this metapage check is only effective retrospectively, once
we recognize and fix problems in an opclass. Clearly there will be
exactly one case like that post-fix (interval_ops is at least the only
affected core code opfamily), so why not point that out directly with
a HINT? A HINT could go a long way towards putting the problem in
context, without really adding a special case, and without any real
question of users being misled.

> If users want to reduce their exposure now, they could do "ALTER INDEX ... SET
> (deduplicate_items = off)" and then REINDEX any indexes already failing
> "pg_amcheck --heapallindexed". allequalimage will remain wrong but have no
> ill effects beyond making amcheck fail. Another REINDEX after the update
> would let amcheck pass.

Even when "deduplicate_items = off", that just means that the nbtree
code won't apply further deduplication passes going forward (until
such time as deduplication is reenabled). It doesn't really mean that
this problem can't exist. OTOH it's easy to detect affected indexes
using SQL. So this is one case where telling users to REINDEX really
does seem like the best thing (as opposed to something we say because
we're too lazy to come up with nuanced, practical guidance).

--
Peter Geoghegan