Retry creating unique index #613
Conversation
agedemenli
force-pushed
the
high-load-unique-index
branch
from
3b8f549 to
64815e8
Compare
pkg/migrations/op_set_unique.go
Outdated
| } | ||
|
|
||
| // Make sure Postgres is done creating the index | ||
| isInProgress := true |
There was a problem hiding this comment.
Using a time.Ticker here is more idiomatic to Golang: https://pkg.go.dev/time#Ticker
We could write something like this (it might not compile, but this is the gist of what I mean):
ticker := time.NewTicker(500 * time.Millisecond)
defer ticker.Stop()
var err error
isIndexInProgress := true
for isIndexInProgress {
select {
case t := <-ticker.C:
isInProgress, err = isIndexInProgress(ctx, conn, s.Name, o.Name)
if err != nil {
return nil, err
}
}
}
pkg/migrations/op_set_unique.go
Outdated
| @@ -83,3 +115,48 @@ func addUniqueIndex(ctx context.Context, conn db.DB, table, column, name string) | |||
|
|
|||
| return err | |||
| } | |||
|
|
|||
| func isIndexInProgress(ctx context.Context, conn db.DB, schemaname string, indexname string) (bool, error) { | |||
| var isInProgress bool | |||
There was a problem hiding this comment.
Nit: please move inProgress to line 135 (before using it in ScanFirstValue).
pkg/migrations/op_set_unique.go
Outdated
| } | ||
|
|
||
| func isIndexValid(ctx context.Context, conn db.DB, schemaname string, indexname string) (bool, error) { | ||
| var isValid bool |
There was a problem hiding this comment.
Nit: please move isValid to line 157 (before using it in ScanFirstValue).
pkg/migrations/op_set_unique.go
Outdated
| // Add a unique index to the new column | ||
| if err := addUniqueIndex(ctx, conn, table.Name, column.Name, o.Name); err != nil { | ||
| return nil, fmt.Errorf("failed to add unique index: %w", err) | ||
| for retryCount := 5; retryCount > 0; retryCount-- { |
There was a problem hiding this comment.
This format is more idiomatic:
| for retryCount := 5; retryCount > 0; retryCount-- { | |
| for retryCount := 0; retryCount < 5; retryCount++ { |
There was a problem hiding this comment.
This looks good 👍
There are other places where we create indexes concurrently that might fail in a similar way; #485 lists another (under Reproduction 2), so we might want to extract this to something that we can re-use for these other cases too in later PRs
pkg/migrations/index.go
Outdated
| return err | ||
| } | ||
| } | ||
| ticker.Stop() |
There was a problem hiding this comment.
Please move ticker.Stop after the line where you initialize it, and add defer:
ticker := time.NewTicker(500 * time.Millisecond)
defer ticker.Stop()With this change we make sure that if isIndexInProgress returns an error, the ticker is stopped.
| continue | ||
| } | ||
| if duplicatedMember, constraintColumns := d.stmtBuilder.allConstraintColumns(uc.Columns, colNames...); duplicatedMember { | ||
| if err := createUniqueIndexConcurrently(ctx, d.conn, "", DuplicationName(uc.Name), d.stmtBuilder.table.Name, constraintColumns); err != nil { |
Introduce a retry mechanism for creating unique indexes.
First, create unique index concurrently. Then wait until Postgres is done with the index creation, periodically checking the view
pg_stat_progress_create_index. Once it is done, lookup pg_index to see if the index is validated or not. If it is, we are good to go. If not, drop the index and try again.Since we are running select queries on
pg_stat_progress_create_indexandpg_indexand we are expecting a real output to see the progress, this approach doesn't work with fake db. I have added hardcoded responses for the fake db scenario, so that we can safely process migrations to update virtual schema as well.Not sure if this is a good solution or not. Open for discussions.
Tested manually with high load, while 10m rows being inserted. But I wonder if there's a way to add a test for the "high load" scenario.