Repository navigation
fix(postgres,pgx): use pg_try_advisory_lock with exponential backoff in Lock - #1451
Open
amirsalarsafaei wants to merge 5 commits into
Open
amirsalarsafaei wants to merge 5 commits into
amirsalarsafaei wants to merge 5 commits into
Conversation
pg_advisory_lock blocks until the lock is free, so concurrent migrations can hang forever (golang-migrate#960). Poll pg_try_advisory_lock instead and retry with an exponential backoff. The max retry interval can be set with x-lock-retry-max-interval. Rebased from golang-migrate#962. Fixes golang-migrate#960
Also return database errors right away instead of retrying them, and use the default retry intervals when Config.Locking is not set. Without that, WithInstance and WithConnection callers retried with no delay at all.
Same change as for the postgres driver, for pgx and pgx/v5. For pgx (v4) this only affects the advisory lock strategy.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retry boundary handling and scheduling-dependent regression tests need correction.
Review effort: Balanced
Findings: 6
Open (6)
What changed in this PR
Reworks PostgreSQL advisory locking to avoid concurrent migration deadlocks.
Changes:
- Uses nonblocking advisory locks with exponential backoff.
- Adds retry configuration and documentation across PostgreSQL drivers.
- Adds concurrent migration and cancellation tests.
| File | Description |
|---|---|
database/postgres/README.md |
Documents retry configuration. |
database/postgres/postgres.go |
Adds retrying, context-aware locking. |
database/postgres/postgres_test.go |
Adds concurrency and cancellation tests. |
database/pgx/README.md |
Documents retry configuration. |
database/pgx/pgx.go |
Adds advisory-lock retries. |
database/pgx/pgx_test.go |
Adds concurrent migration coverage. |
database/pgx/v5/README.md |
Documents retry configuration. |
database/pgx/v5/pgx.go |
Adds advisory-lock retries. |
database/pgx/v5/pgx_test.go |
Adds concurrent migration coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… initial one Lock retry defaults are now applied in one place. A max interval below the initial interval is raised to it, otherwise the first retry would already wait longer than the max. This also applies to x-lock-retry-max-interval, where such values used to be ignored.
Up and Drop take the lock separately, so a worker could get the lock in between and fail with ErrNoChange. Run only Up, expect exactly one worker to apply the migrations, and stop calling t.Fatal from the worker goroutines.
|
coverage: 55.422%. first build — amirsalarsafaei:postgres-try-advisory-lock into golang-migrate:master |
Author
|
@Fontinalis Hi, Could you take a look at this? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #960, picks up #962.
#962 went stale and no longer applied on master, so I rebased it and squashed it into a single commit. @AkuSilvenius is still the author. On top of it:
ctx.Err()once the ctx passed toWithConnectionis done.OpenandWithInstanceusecontext.Background(), so nothing changes for them, and thedatabase.Driverinterface stays the same.Lockretry forever.Config.Lockingisn't set, the default retry intervals are used. Before,WithInstance/WithConnectioncallers got a 0 interval andpg_try_advisory_lockran in a tight loop.pgxandpgx/v5, includingx-lock-retry-max-interval. Inpgxit only affects the advisory lock strategy.Tests:
WithConnectionctx gets canceled while another connection holds the lock.The postgres, pgx and pgx/v5 test suites pass locally on postgres 14 to 18.