Date: Wed, 11 Oct 2023 11:38:35 -0700
From: Noah Misch <noah@leadboat.com>
To: Peter Geoghegan <pg@bowt.ie>, pgsql-hackers@postgresql.org
Subject: Re: interval_ops shall stop using btequalimage (deduplication)
Message-ID: <20231011183835.32@rfd.leadboat.com>
References: <20231011013317.22.nmisch@google.com>
 <CAH2-Wzm_OFH4Aip3uinAJ-AoBgX0wGjB9XBhrhkTnjCoZ_i8NA@mail.gmail.com>
 <20231011032908.35@rfd.leadboat.com>
 <CAH2-WzmDbyMj_7Ev5vPmt7UaKjuRg6m79sRguTjehyZtuBPMgw@mail.gmail.com>
 <CAH2-WzmCppEF8UFUVLbL3SXLD3UwB-5sDV2YC99soN6O0ScWpg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAH2-WzmCppEF8UFUVLbL3SXLD3UwB-5sDV2YC99soN6O0ScWpg@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20231011183835.32%40rfd.leadboat.com>
Precedence: bulk

On Tue, Oct 10, 2023 at 09:35:45PM -0700, Peter Geoghegan wrote:
> On Tue, Oct 10, 2023 at 8:51 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > I don't see any reason to delay committing your fix. The issue that
> > you've highlighted is exactly the kind of issue that I anticipated
> > might happen at some point. This seems straightforward.
> 
> BTW, we don't need to recommend the heapallindexed option in the
> release notes. Calling bt_check_index() will reliably indicate that
> corruption is present when called against existing interval_ops
> indexes once your fix is in. Simply having an index whose metapage's
> allequalimage field is spuriously set to true will be recognized as
> corruption right away. Obviously, this will be no less true with an
> existing interval_ops index that happens to be completely empty.

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.  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."  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?

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.