Feedback-ID: ie3de48e3:Fastmail
Date: Tue, 15 Sep 2026 17:30:54 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Robert Treat <rob@xzilla.net>
Cc: shihao zhong <zhong950419@gmail.com>,
	Zsolt Parragi <zsolt.parragi@percona.com>,
 Christophe Pettus <xof@thebuild.com>,
	pgsql-hackers@lists.postgresql.org,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	rmt@lists.postgresql.org
Subject: Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
Message-ID: <aqkXyUZFk__dKqvm@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CABV9wwOfL+5YfCK8mKzgerQKFPuLPgHg8Arsqp0nDTzP6ug0Dg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aqkXyUZFk__dKqvm%40alvherre.pgsql>
Precedence: bulk

On 2026-Sep-14, Robert Treat wrote:

> The patch looks right to me, and I think I am +1 for this generally,
> though I would note that we are widening the scope here such that
> indexes that could succeed with a rebuild would now cause an error, so
> we're a little less functional though a little more behaviorally
> consistent.

Yeah.  I considered the idea of adding a REPACK option like
"rebuild_invalid_indexes=on" that would attempt to rebuild rather than
failing outright, at the user's risk.  Not sure it's worth the trouble.

I have pushed this change now, without the test.  I did throw in a short
doc update.

One thing I didn't want to say in said doc update, is that while you can
do a REINDEX of the non-validated index prior to REPACK, it's rather a
complete waste of time, because REPACK has to rebuild that index again.
It's probably better to DROP the index, then REPACK, then do CREATE
INDEX CONCURRENTLY.  If we didn't cause REPACK to error out in the
presence of a buildable invalid index, then REPACK could rebuild the
index just fine.  I'm happy to listen to your operationally-experienced
opinion on this.

> This does feel hacky, since we're being manipulative rather than
> testing a real scenario.

Yeah.

> I think what we want would be to follow the lead of
> src/test/modules/injection_points/sql/reindex_conc.sql?

Do you mean creating test invalid indexes by way of using injection
points to interrupt creation in the various phases?  I don't see
reindex_conc.sql doing that.  But also, we run these tests in
test_decoding because it's the test suite that is certain to have
wal_level=logical; but if we wanted to also require injection_points,
the changes in meson.build / Makefile get more involved.  I didn't find
a test suite that has conditional tests depending on injection_points.
src/test/modules/authentication does, but for a TAP test, not a plain
regress or isolation test.  It is surely just a SMOP ...

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)