REPACK (CONCURRENTLY) doesn't handle invalid indexes

Hello,

On 2026-Jul-21, Zsolt Parragi wrote:

While testing REPACK, I noticed that REPACK (CONCURRENTLY) fails with
invalid unique indexes, while the non-concurrent version handles them.

Thanks for testing!

The attached patch mirrors this handling for CONCURRENTLY: constraints
won't be enforced for invalid indexes.

Wouldn't it make more sense to just ignore invalid indexes and not build
anything at all for them?

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Wouldn't it make more sense to just ignore invalid indexes and not build
anything at all for them?

I followed what REPACK/VACUUM FULL does - it similarly rebuilds invalid indexes, shouldn't we keep the two consistent?

And by skipping you mean that we should create an empty index instead when repack sees an invalid index?

One argument for that is that the current code only handles unique constraint violation, while there's a preexisting issue that these commands all fail with other index failures, except REINDEX TABLE CONCURRENTLY, e.g.:

CREATE TABLE t (i int PRIMARY KEY, j int);
INSERT INTO t VALUES (1, 0), (2, 1);
CREATE INDEX CONCURRENTLY t_expr ON t ((1/j));

VACUUM FULL t;
CLUSTER t USING t_pkey;
REINDEX TABLE t;
ALTER TABLE t ALTER COLUMN j TYPE bigint;
REINDEX TABLE CONCURRENTLY t; -- only this works

If we would skip all invalid indexes, that could solve this too (except for the ALTER TABLE).

I wanted to raise this issue separately, as this seems to be preexisting in earlier versions, not a new bug. (and a more complex discussion than "repack and repack (concurrently) should behave similarly")

On 2026-Jul-27, Zsolt Parragi wrote:

Wouldn't it make more sense to just ignore invalid indexes and not build
anything at all for them?

I followed what REPACK/VACUUM FULL does - it similarly rebuilds
invalid indexes, shouldn't we keep the two consistent?

I don't know. Maybe rebuilding invalid indexes is pointless. Don't you
think so? An invalid index can never be turned valid, so why spend
effort in building it at all?

And by skipping you mean that we should create an empty index instead
when repack sees an invalid index?

I mean we should just ignore all invalid indexes.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

I don't know. Maybe rebuilding invalid indexes is pointless. Don't you
think so? An invalid index can never be turned valid, so why spend
effort in building it at all?

After a bit of testing: it isn't used by reads, but it is used by uniqueness checks:

CREATE TABLE u (k int);
INSERT INTO u VALUES (1);
CREATE UNIQUE INDEX u_k_uq ON u(k);

SET allow_system_table_mods = on;
UPDATE pg_index SET indisvalid = false WHERE indexrelid = 'u_k_uq'::regclass;
RESET allow_system_table_mods;

INSERT INTO u VALUES (1); -- it exists, fails
INSERT INTO u VALUES (2); -- new row, succeeds, gets indexed
INSERT INTO u VALUES (2); -- it exists, fails

If we empty it, as I suggested, uniqueness checks will only work for new records after repack, it will ignore everything preexisting.
If we leave it alone, we get completely bogus results and also sporadic errors like:

ERROR: could not read blocks 344..344 in file "base/5/16401": read only 0 of 8192 bytes

The leave it alone approach only works if indisready is also true, which would be the case for my repro above, but not for some other scenarios. I also have a repro which results in isvalid=false, inready=true where we should rebuild the index.

I'll create a patch for that approach, but it is a bit more complex than v1.

I'll create a patch for that approach, but it is a bit more complex than v1.

It turned out not to be that complex, attached v2.

One thing to point out is that this is a slight behavior change for
REINDEX TABLE. We could avoid that, but the current behavior doesn't
seem to be documented anywhere, and this version seems more consistent
to me.

Does this one deserve a mention on the open items wiki [0]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items?

[0]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

--
nathan

Hello,

At Thu, 13 Aug 2026 22:37:25 +0100, Zsolt Parragi <zsolt.parragi@percona.com> wrote in

I'll create a patch for that approach, but it is a bit more complex than v1.

It turned out not to be that complex, attached v2.

One thing to point out is that this is a slight behavior change for
REINDEX TABLE. We could avoid that, but the current behavior doesn't
seem to be documented anywhere, and this version seems more consistent
to me.

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.

I understand that an invalid index with indisready set cannot simply
be ignored by normal DML. As I understand it, indisready represents an
intermediate state in a concurrent index build, allowing INSERT/UPDATE
maintenance to start before the index becomes available for
queries. Therefore, if a concurrent index build fails at that stage,
an index with indisready = true and indisvalid = false can be left
behind.

However, I think there is a distinction between DML having to continue
maintaining such an index according to indisready and a later,
unrelated DDL command rebuilding it as if it were a normal index.

An invalid index left behind by a failed concurrent index build cannot
be used for queries, and is normally either dropped or explicitly
rebuilt with REINDEX. The fact that indisready is true does not mean
that the index is valid for normal use; it can simply mean that
maintenance had already been enabled as part of the concurrent
operation before it failed.

For that reason, I am somewhat uncomfortable with REPACK implicitly
rebuilding such an index. REPACK is not a command for repairing
indexes, so wouldn't it be more natural to reject the operation if an
invalid index exists and require the user to DROP or REINDEX it first,
rather than trying to reproduce or repair that state as part of
REPACK?

I understand that non-concurrent REPACK and VACUUM FULL currently
rebuild invalid indexes. However, that seems to be a consequence of
rebuilding all indexes as part of the heap rewrite, and I am not sure
that this behavior should necessarily define the semantics for REPACK
CONCURRENTLY. In fact, Alvaro's point also makes me wonder about the
existing behavior itself. I wonder whether non-concurrent REPACK or
VACUUM FULL should implicitly rebuild such an index as a side effect
of an operation with a different purpose.

So rather than deciding which invalid indexes to rebuild based on
indisready, as in v2, perhaps we should first decide whether a
relation containing an invalid index should be considered a valid
input for REPACK at all.

Regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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.

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.

Hi

On Wed, Aug 26, 2026 at 3:31 PM Zsolt Parragi <zsolt.parragi@percona.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

On Aug 26, 2026, at 00:31, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
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.

I think your statement has its own answer embedded in it: It's acceptable for REPACK, a brand-new command, to have different behavior in the presence of invalid indexes specifically because it would be a behavior change if we pushed that change to other commands.

On Mon, 31 Aug 2026, Christophe Pettus <xof@thebuild.com> wrote:

I think your statement has its own answer embedded in it: It's acceptable for REPACK, a brand-new command, to have different behavior in the presence of invalid indexes specifically because it would be a behavior change if we pushed that change to other commands.

But that's not exactly what happens here: currently REPACK and REPACK (CONCURRENTLY) behave differently in the presence of invalid indexes, and that is definitely a bug. There's no issue with the non concurrent REPACK, that behaves like the other commands do. Making REPACK (CONCURRENTLY) handle indexes the same way as other commands currently do resolves the difference, and doesn't cause any behavior change for other commands.

On Aug 31, 2026, at 16:08, Zsolt Parragi <zsolt.parragi@percona.com> wrote:

On Mon, 31 Aug 2026, Christophe Pettus <xof@thebuild.com> wrote:

I think your statement has its own answer embedded in it: It's acceptable for REPACK, a brand-new command, to have different behavior in the presence of invalid indexes specifically because it would be a behavior change if we pushed that change to other commands.

But that's not exactly what happens here: currently REPACK and REPACK
(CONCURRENTLY) behave differently in the presence of invalid indexes,
and that is definitely a bug.

"Differently" as such isn't a bug. "Incorrectly" would be. If REPACK AND REPACK CONCURRENTLY simply refused to run on a table with invalid indexes, that's minimally intrusive and fixes the bug. I agree that the ideal situation is that they work tables with invalid indexes, but I don't consider it a bug if they just refused to.

Hi Zsolt, and team,

Just a housekeeping note as commitfest manager: this thread had two
identical commitfest entries, #7039 and #7040, both created in the same
second -- almost certainly a double submit of the registration form.

I have withdrawn #7040 as a duplicate. The surviving entry is:

https://commitfest.postgresql.org/patch/7039/

Thanks,
Shihao

Moving thread to -hackers.

On 2026-Aug-31, Zsolt Parragi wrote:

On Mon, 31 Aug 2026, Christophe Pettus <xof@thebuild.com> wrote:

I think your statement has its own answer embedded in it: It's
acceptable for REPACK, a brand-new command, to have different
behavior in the presence of invalid indexes specifically because it
would be a behavior change if we pushed that change to other
commands.

But that's not exactly what happens here: currently REPACK and REPACK
(CONCURRENTLY) behave differently in the presence of invalid indexes,
and that is definitely a bug.

Yes, but I think the question is in which direction should we fix said
bug. My preference is to go for Kyotaro's suggestion: have both REPACK
and REPACK (CONCURRENTLY) raise an error with an invalid index, asking
the user to drop it.

Would anybody oppose that?

Maybe in pg20, barring complaints against this, we can propagate the
same behavior to CLUSTER and VACUUM FULL. (But that obviously need more
discussion.)

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/