Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes

Hi

On Wed, Aug 26, 2026 at 3:31 PM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> > I think Alvaro's point about whether invalid indexes should be rebuilt
> > in the first place is worth considering further. In fact, I wonder
> > whether REPACK should accept a relation containing an invalid index at
> > all.
>
> Not rebuilding it / emptying it isn't really an option, as I showed an
> example in my earlier emails, not rebuilding it results in bogus
> checks and statements failing with file read errors, if we empty it it
> results in additional constraint violations.
>
> Not allowing these commands (consistently) to work on tables with
> invalid indexes is an option, but then that should be consistent
> across all similar commands, and it will be a behavior change for
> normal vacuum too.
>
> Actually after I looked at this again after Nathan's email yesterday,
> I realized that even v2 causes a regression (or lets call it a
> behavior change at least), most likely v1 is a better solution.

I tested v2 on master as of 2f4df67f5d0 (cassert build) and can confirm
both of your scenarios. The underlying rule is in reindex_index():
after a successful rebuild it marks an invalid index valid again unless
it had to skip a uniqueness check, and skipped_constraint is only set
for unique and exclusion indexes (index.c:3847). So today a heap
rewrite fully repairs any *non-unique* invalid index -- VACUUM FULL,
CLUSTER and TRUNCATE included -- and REFRESH MATERIALIZED VIEW, which
passes check_constraints = true to finish_heap_swap() (matview.c),
repairs unique ones as well. In both of your scenarios the index ends
up valid on an unpatched build and stays invalid with v2, with a
WARNING. So the behavior change in v2 is wider than its commit message
suggests; it is not limited to REINDEX TABLE.

While testing v2 I ran into two more reindex_relation() callers that
hadn't come up in the thread:

- TRUNCATE goes through reindex_relation() too (tablecmds.c,
"Reconstruct the indexes to match"), so with v2 every TRUNCATE of
such a table emits the WARNING and leaves the index storage alone,
although rebuilding an index over an empty heap cannot fail:

CREATE TABLE tr (i int, j int);
INSERT INTO tr VALUES (1, 0);
CREATE INDEX CONCURRENTLY tr_expr ON tr ((1/j)); -- fails
TRUNCATE tr;
WARNING: skipping invalid index "public.tr_expr"
HINT: Use DROP INDEX or REINDEX INDEX.

- The new check sits in front of the existing invalid-TOAST-index
check, so a toast index that a failed REINDEX CONCURRENTLY left
neither valid nor ready gets the generic message above instead of
"cannot reindex invalid index ... on TOAST table, skipping" -- and
the REINDEX INDEX half of the new hint fails on toast indexes:

REINDEX INDEX pg_toast.pg_toast_16515_index_ccnew;
ERROR: cannot reindex invalid index on TOAST table

So +1 to going with v1 for 19 -- it still applies cleanly to the current
master and fixes the original failure here.

>
> Consider the following scenario:
>
> CREATE TABLE orders (id int PRIMARY KEY, price int);
> INSERT INTO orders VALUES (1, 10), (2, 0), (3, 20);
> -- currently fails with division by zero
> CREATE INDEX CONCURRENTLY orders_margin ON orders ((100/price));
> -- removing bad data
> DELETE FROM orders WHERE price = 0;
> -- repairs the index
> VACUUM FULL orders;
>
> or another less visible example is REFRESH MATERIALIZED VIEW:
>
> CREATE MATERIALIZED VIEW mv AS SELECT * FROM src;
> REFRESH MATERIALIZED VIEW mv; -- let's say this is a daily/hourly cron
> job or something like that
> CREATE INDEX CONCURRENTLY mv_margin ON mv ((100/price)); -- fails
> DELETE FROM src WHERE price = 0;
> REFRESH MATERIALIZED VIEW mv; -- index now works on master/v1, remains
> invalid in v2
>
> And if I follow your suggestion consistently across all commands about
> treating it as an invalid input the last command should fail in both
> scenarios.
>
> v1 seems to be a better/less risky version to me, especially for 19.
>
>

--
Regards,
Ewan Young