Skip to content

Commit 986ac94

Browse files
authored
reindex numbered concurrent artifacts (#1297)
Postgres can leave invalid `_ccnew` and `_ccold` indexes after a failed concurrent reindex. When artifacts already existed and Postgres had to append digits to keep names unique, River only checked the unnumbered forms and could keep trying the same reindex, adding more large artifacts. Add a driver-level catalog lookup for artifacts belonging to a configured index. It matches the exact configured index name followed by `_ccnew` or `_ccold` with optional digits, returns the names for logging and cleanup, and lets stop-time cleanup drop the same numbered artifacts. SQLite keeps no-op semantics because it does not create concurrent reindex artifacts. Extend the maintenance and driver conformance coverage for numbered forms and near-miss names so similar user-created indexes are not treated as PostgreSQL artifacts. Make `riverdbtest.TestSchema`'s `DisableReuse` option use a fresh schema and avoid returning it to the idle pool, which keeps schema-mutating tests from leaking ad hoc indexes into later test cases. Document the user-facing fix in the changelog. Fixes #1296.
1 parent 78ae535 commit 986ac94

13 files changed

Lines changed: 316 additions & 76 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ river migrate-get --database-url sqlite:// --version 6 --down > river7.down.sql
4949

5050
- Fix `JobCancel` having no effect on running jobs when using a poll-only driver (e.g. `riverdatabasesql`). The `controlActionCancel` event was silently dropped in `fetchAndRunLoop`'s `queueControlCh` handler instead of being forwarded to `maybeCancelJob`. Note: this fix only works within a single process; cross-process cancels in poll-only setups must wait for the next poll cycle. [PR #1245](https://github.com/riverqueue/river/pull/1245).
5151
- Ensure jobs that return a custom timeout of -1 (no timeout) are never rescued. [PR #1288](https://github.com/riverqueue/river/pull/1288).
52+
- Detect numbered PostgreSQL `REINDEX INDEX CONCURRENTLY` artifacts like `_ccnew1` and `_ccold2` so the reindexer does not keep accumulating failed artifact indexes. Fixes [#1296](https://github.com/riverqueue/river/issues/1296). [PR #1297](https://github.com/riverqueue/river/pull/1297).
5253

5354
## [0.39.0] - 2026-06-03
5455

internal/maintenance/reindexer.go

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,8 @@ func (s *Reindexer) reindexOne(ctx context.Context, indexName string) (bool, err
221221
// exist before trying to reindex. When using `CONCURRENTLY`, Postgres
222222
// creates a new index suffixed with `_ccnew` before swapping it in as the
223223
// new index. The existing index is renamed `_ccold` before being dropped
224-
// concurrently.
224+
// concurrently. If multiple failed artifacts exist, Postgres may add a
225+
// numeric suffix like `_ccnew1` or `_ccold2` to keep names unique.
225226
//
226227
// If one of these artifacts exists, it probably means that a previous
227228
// reindex attempt timed out, and attempting to reindex again is likely
@@ -234,16 +235,15 @@ func (s *Reindexer) reindexOne(ctx context.Context, indexName string) (bool, err
234235
//
235236
// https://www.postgresql.org/docs/current/sql-reindex.html#SQL-REINDEX-CONCURRENTLY
236237
if !s.skipReindexArtifactCheck {
237-
for _, reindexArtifactName := range []string{indexName + "_ccnew", indexName + "_ccold"} {
238-
reindexArtifactExists, err := s.exec.IndexExists(ctx, &riverdriver.IndexExistsParams{Index: reindexArtifactName, Schema: s.Config.Schema})
239-
if err != nil {
240-
return false, err
241-
}
242-
if reindexArtifactExists {
243-
s.Logger.WarnContext(ctx, s.Name+": Found reindex artifact likely resulting from previous partially completed reindex attempt; skipping reindex",
244-
slog.String("artifact_name", reindexArtifactName), slog.String("index_name", indexName), slog.Duration("timeout", s.Config.Timeout))
245-
return false, nil
246-
}
238+
reindexArtifactNames, err := s.exec.IndexReindexArtifacts(ctx, &riverdriver.IndexReindexArtifactsParams{Index: indexName, Schema: s.Config.Schema})
239+
if err != nil {
240+
return false, err
241+
}
242+
243+
if len(reindexArtifactNames) > 0 {
244+
s.Logger.WarnContext(ctx, s.Name+": Found reindex artifact likely resulting from previous partially completed reindex attempt; skipping reindex",
245+
slog.Any("artifact_names", reindexArtifactNames), slog.String("index_name", indexName), slog.Duration("timeout", s.Config.Timeout))
246+
return false, nil
247247
}
248248
}
249249

@@ -268,7 +268,12 @@ func (s *Reindexer) reindexOne(ctx context.Context, indexName string) (bool, err
268268

269269
s.Logger.InfoContext(ctx, s.Name+": Signaled to stop during index build; attempting to clean up concurrent artifacts")
270270

271-
for _, reindexArtifactName := range []string{indexName + "_ccnew", indexName + "_ccold"} {
271+
reindexArtifactNames, err := s.exec.IndexReindexArtifacts(ctx, &riverdriver.IndexReindexArtifactsParams{Index: indexName, Schema: s.Config.Schema})
272+
if err != nil {
273+
s.Logger.ErrorContext(ctx, s.Name+": Error listing reindex artifacts", slog.String("error", err.Error()))
274+
}
275+
276+
for _, reindexArtifactName := range reindexArtifactNames {
272277
if err := s.exec.IndexDropIfExists(ctx, &riverdriver.IndexDropIfExistsParams{Index: reindexArtifactName, Schema: s.Config.Schema}); err != nil {
273278
s.Logger.ErrorContext(ctx, s.Name+": Error dropping reindex artifact", slog.String("artifact_name", reindexArtifactName), slog.String("error", err.Error()))
274279
}

internal/maintenance/reindexer_test.go

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,13 @@ func TestReindexer(t *testing.T) {
6464
schema string
6565
}
6666

67-
setup := func(t *testing.T) (*Reindexer, *testBundle) {
67+
setupWithOpts := func(t *testing.T, testSchemaOpts *riverdbtest.TestSchemaOpts) (*Reindexer, *testBundle) {
6868
t.Helper()
6969

7070
var (
7171
dbPool = riversharedtest.DBPool(ctx, t)
7272
driver = riverpgxv5.New(dbPool)
73-
schema = riverdbtest.TestSchema(ctx, t, driver, nil)
73+
schema = riverdbtest.TestSchema(ctx, t, driver, testSchemaOpts)
7474
)
7575

7676
bundle := &testBundle{
@@ -100,6 +100,12 @@ func TestReindexer(t *testing.T) {
100100
return svc, bundle
101101
}
102102

103+
setup := func(t *testing.T) (*Reindexer, *testBundle) {
104+
t.Helper()
105+
106+
return setupWithOpts(t, nil)
107+
}
108+
103109
runImmediatelyThenOnceAnHour := func() func(time.Time) time.Time {
104110
alreadyRan := false
105111
return func(t time.Time) time.Time {
@@ -141,7 +147,13 @@ func TestReindexer(t *testing.T) {
141147
t.Run("ReindexSkippedWithReindexArtifact", func(t *testing.T) {
142148
t.Parallel()
143149

144-
svc, bundle := setup(t)
150+
svc, bundle := setupWithOpts(t, &riverdbtest.TestSchemaOpts{DisableReuse: true})
151+
152+
mockExec := newReindexerExecutorMock(bundle.exec)
153+
mockExec.indexReindexFunc = func(ctx context.Context, params *riverdriver.IndexReindexParams) error {
154+
return nil
155+
}
156+
svc.exec = mockExec
145157

146158
requireReindexOne := func(indexName string) bool {
147159
didReindex, err := svc.reindexOne(ctx, indexName)
@@ -151,20 +163,20 @@ func TestReindexer(t *testing.T) {
151163

152164
indexName := svc.Config.IndexNames[0]
153165

154-
// With a `_ccnew` index in place, the reindexer refuses to run.
155-
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s_ccnew ON %s.river_job (id)", indexName, bundle.schema)))
156-
require.False(t, requireReindexOne(indexName))
157-
158-
// With the index dropped again, reindexing can now occur.
159-
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("DROP INDEX %s.%s_ccnew", bundle.schema, indexName)))
160-
require.True(t, requireReindexOne(indexName))
161-
162-
// `_ccold` also prevents reindexing.
163-
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s_ccold ON %s.river_job (id)", indexName, bundle.schema)))
164-
require.False(t, requireReindexOne(indexName))
166+
for _, artifactSuffix := range []string{"_ccnew", "_ccnew1", "_ccnew31", "_ccold", "_ccold1"} {
167+
artifactName := indexName + artifactSuffix
168+
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", artifactName, bundle.schema)))
169+
require.False(t, requireReindexOne(indexName))
170+
require.NoError(t, bundle.exec.IndexDropIfExists(ctx, &riverdriver.IndexDropIfExistsParams{Index: artifactName, Schema: bundle.schema}))
171+
}
165172

166-
// And with `_ccold` dropped, reindexing can proceed.
167-
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("DROP INDEX %s.%s_ccold", bundle.schema, indexName)))
173+
for _, artifactSuffix := range []string{"_ccnewa", "_ccnew_1", "_ccoldx", "_ccold_1", "x_ccnew1"} {
174+
artifactName := indexName + artifactSuffix
175+
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", artifactName, bundle.schema)))
176+
t.Cleanup(func() {
177+
require.NoError(t, bundle.exec.IndexDropIfExists(ctx, &riverdriver.IndexDropIfExistsParams{Index: artifactName, Schema: bundle.schema}))
178+
})
179+
}
168180
require.True(t, requireReindexOne(indexName))
169181
})
170182

@@ -249,7 +261,7 @@ func TestReindexer(t *testing.T) {
249261
t.Run("ReindexDeletesArtifactsWhenCancelledWithStop", func(t *testing.T) {
250262
t.Parallel()
251263

252-
svc, bundle := setup(t)
264+
svc, bundle := setupWithOpts(t, &riverdbtest.TestSchemaOpts{DisableReuse: true})
253265
svc.skipReindexArtifactCheck = true
254266

255267
requireIndexExists := func(indexName string) bool {
@@ -259,16 +271,25 @@ func TestReindexer(t *testing.T) {
259271
}
260272

261273
var (
262-
indexName = svc.Config.IndexNames[0]
263-
indexNameNew = indexName + "_ccnew"
264-
indexNameOld = indexName + "_ccold"
274+
indexName = svc.Config.IndexNames[0]
275+
indexNameNew = indexName + "_ccnew"
276+
indexNameNewNumber = indexName + "_ccnew1"
277+
indexNameNonArtifact = indexName + "_ccnewa"
278+
indexNameOld = indexName + "_ccold"
279+
indexNameOldNumber = indexName + "_ccold2"
265280
)
266281

267282
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", indexNameNew, bundle.schema)))
283+
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", indexNameNewNumber, bundle.schema)))
284+
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", indexNameNonArtifact, bundle.schema)))
268285
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", indexNameOld, bundle.schema)))
286+
require.NoError(t, bundle.exec.Exec(ctx, fmt.Sprintf("CREATE INDEX %s ON %s.river_job (id)", indexNameOldNumber, bundle.schema)))
269287

270288
require.True(t, requireIndexExists(indexNameNew))
289+
require.True(t, requireIndexExists(indexNameNewNumber))
290+
require.True(t, requireIndexExists(indexNameNonArtifact))
271291
require.True(t, requireIndexExists(indexNameOld))
292+
require.True(t, requireIndexExists(indexNameOldNumber))
272293

273294
{
274295
// Pre-cancel context to simulate a reindexer being stopped while
@@ -286,7 +307,10 @@ func TestReindexer(t *testing.T) {
286307
}
287308

288309
require.False(t, requireIndexExists(indexNameNew))
310+
require.False(t, requireIndexExists(indexNameNewNumber))
311+
require.True(t, requireIndexExists(indexNameNonArtifact))
289312
require.False(t, requireIndexExists(indexNameOld))
313+
require.False(t, requireIndexExists(indexNameOldNumber))
290314
})
291315

292316
t.Run("StopsImmediately", func(t *testing.T) {

riverdbtest/riverdbtest.go

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,10 @@ var (
5151
// TestSchemaOpts are options for TestSchema. Most of the time these can be left
5252
// as nil.
5353
type TestSchemaOpts struct {
54-
// DisableReuse specifies that schema will not be checked in for reuse at
55-
// the end of tests. This is desirable in certain like cases like where a
56-
// test case is making modifications to schema.
54+
// DisableReuse specifies that a schema will not be checked out from the
55+
// idle schema pool or checked in for reuse at the end of tests. This is
56+
// desirable in cases like where a test case is making modifications to
57+
// schema.
5758
//
5859
// Not being able to reuse the schema introduces overhead to tests because
5960
// it means more schemas will need to be generated to replace one not
@@ -320,44 +321,47 @@ func TestSchema[TTx any](ctx context.Context, tb testutil.TestingTB, driver rive
320321
return userFacingSchema
321322
}
322323

323-
// See if there are any idle schemas that were previously generated during
324-
// this run and have since been checked back into the pool. If so, pop it
325-
// off and run cleanup on it. If not, continue on to generating a new schema
326-
// below. This function never blocks, so we'll prefer generating extra
327-
// schemas rather than optimizing amongst a minimal set that's already there.
328-
if schema := func() string {
329-
idleSchemasMu.Lock()
330-
defer idleSchemasMu.Unlock()
331-
332-
linesIdleSchemas := idleSchemas[databaseAndLinesKey]
324+
if !opts.DisableReuse {
325+
// See if there are any idle schemas that were previously generated
326+
// during this run and have since been checked back into the pool. If
327+
// so, pop it off and run cleanup on it. If not, continue on to
328+
// generating a new schema below. This function never blocks, so we'll
329+
// prefer generating extra schemas rather than optimizing amongst a
330+
// minimal set that's already there.
331+
if schema := func() string {
332+
idleSchemasMu.Lock()
333+
defer idleSchemasMu.Unlock()
334+
335+
linesIdleSchemas := idleSchemas[databaseAndLinesKey]
336+
337+
if len(linesIdleSchemas) < 1 {
338+
return ""
339+
}
333340

334-
if len(linesIdleSchemas) < 1 {
335-
return ""
336-
}
341+
schema := linesIdleSchemas[0]
342+
idleSchemas[databaseAndLinesKey] = linesIdleSchemas[1:]
343+
return schema
344+
}(); schema != "" {
345+
// Should be called BEFORE maybeProcurePool. maybeProcurePool may open a
346+
// pool, and in case it does, we want a cleanup in it that closes the pool
347+
// to run before this cleanup hook that checks the test schema back in.
348+
// Cleanup is FILO, so clean up must appear first to run last.
349+
addCleanupHook(schema)
350+
351+
var (
352+
start = time.Now()
353+
userFacingSchema = maybeProcurePool(schema)
354+
)
355+
356+
if len(truncateTables) > 0 {
357+
require.NoError(tb, driver.GetExecutor().TableTruncate(ctx, &riverdriver.TableTruncateParams{Schema: userFacingSchema, Table: truncateTables}))
358+
}
337359

338-
schema := linesIdleSchemas[0]
339-
idleSchemas[databaseAndLinesKey] = linesIdleSchemas[1:]
340-
return schema
341-
}(); schema != "" {
342-
// Should be called BEFORE maybeProcurePool. maybeProcurePool may open a
343-
// pool, and in case it does, we want a cleanup in it that closes the pool
344-
// to run before this cleanup hook that checks the test schema back in.
345-
// Cleanup is FILO, so clean up must appear first to run last.
346-
addCleanupHook(schema)
360+
tb.Logf("Reusing idle %s schema %q [user facing: %q] after cleaning in %s [%d generated] [%d reused]",
361+
driver.DatabaseName(), schema, userFacingSchema, time.Since(start), stats.numGenerated.Load(), stats.numReused.Add(1))
347362

348-
var (
349-
start = time.Now()
350-
userFacingSchema = maybeProcurePool(schema)
351-
)
352-
353-
if len(truncateTables) > 0 {
354-
require.NoError(tb, driver.GetExecutor().TableTruncate(ctx, &riverdriver.TableTruncateParams{Schema: userFacingSchema, Table: truncateTables}))
363+
return userFacingSchema
355364
}
356-
357-
tb.Logf("Reusing idle %s schema %q [user facing: %q] after cleaning in %s [%d generated] [%d reused]",
358-
driver.DatabaseName(), schema, userFacingSchema, time.Since(start), stats.numGenerated.Load(), stats.numReused.Add(1))
359-
360-
return userFacingSchema
361365
}
362366

363367
// e.g. river_2025_04_14t22_13_58_schema_10

riverdbtest/riverdbtest_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,22 @@ func TestTestSchemaReuse(t *testing.T) { //nolint:paralleltest
230230
// they also may not be.
231231
requireIdle(t, schema1)
232232
requireIdle(t, schema2)
233+
234+
var (
235+
previouslyIdleSchemasForDisableReuse = append([]string(nil), idleSchemas[databaseAndLinesKey]...)
236+
schema3 string
237+
)
238+
239+
t.Run("DisableReuse", func(t *testing.T) { //nolint:paralleltest
240+
schema3 = TestSchema(ctx, t, driver, &TestSchemaOpts{DisableReuse: true})
241+
requireNotIdle(t, schema3)
242+
require.NotContains(t, previouslyIdleSchemasForDisableReuse, schema3)
243+
})
244+
245+
requireNotIdle(t, schema3)
246+
for _, schema := range previouslyIdleSchemasForDisableReuse {
247+
requireIdle(t, schema)
248+
}
233249
}
234250

235251
func TestPackageFromFunc(t *testing.T) {

riverdriver/river_driver_interface.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ type Executor interface {
206206
//
207207
// API is not stable. DO NOT USE.
208208
IndexReindex(ctx context.Context, params *IndexReindexParams) error
209+
IndexReindexArtifacts(ctx context.Context, params *IndexReindexArtifactsParams) ([]string, error)
209210

210211
JobCancel(ctx context.Context, params *JobCancelParams) (*rivertype.JobRow, error)
211212
JobCountByAllStates(ctx context.Context, params *JobCountByAllStatesParams) (map[rivertype.JobState]int, error)
@@ -863,6 +864,11 @@ type IndexReindexParams struct {
863864
Schema string
864865
}
865866

867+
type IndexReindexArtifactsParams struct {
868+
Index string
869+
Schema string
870+
}
871+
866872
type Schema struct {
867873
Name string
868874
}

riverdriver/riverdatabasesql/internal/dbsqlc/schema.sql.go

Lines changed: 45 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)