Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[/v4] - Add session level advisory lock (postgres only) #493

Merged
merged 6 commits into from Apr 2, 2023

Conversation

mfridman
Copy link
Collaborator

@mfridman mfridman commented Apr 1, 2023

This PR adds a session-level advisory lock with a parallel test.

options := goose.DefaultOptions().
SetDir(migrationsDir).
SetVerbose(testing.Verbose()).
SetLockMode(goose.LockModeAdvisorySession) /* ---------------- advisory session lock mode */
Copy link
Collaborator Author

@mfridman mfridman Apr 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not set, these tests are guaranteed to fail, and we add checkpoints between the 4 test cases.

I ran this test 100 times in a loop with and without the session-level advisory lock and it was:

# with locking
Success: 100 Fail: 0

# without locking
Success: 0 Fail: 100

Jotting down the errors we'd expect below. Now, we're not guaranteed to get all of the errors.

unexpected error: failed to run SQL migration: 00001_a.sql: ERROR: duplicate key value violates unique constraint "pg_type_typname_nsp_index" (SQLSTATE 23505)

// or 

unexpected error: failed to run SQL migration: 00001_a.sql: ERROR: type "owner_type" already exists (SQLSTATE 42710)

// or 

unexpected error: failed to run SQL migration: 00010_j.sql: ERROR: deadlock detected (SQLSTATE 40P01)

That last one is interesting because migration 00010_j.sql has -- +goose NO TRANSACTION set and attempts to run:

CREATE UNIQUE INDEX CONCURRENTLY ON owners(owner_name);

This will always raise a deadlock because of the way concurrent indexes are rebuilt in postrges.


This was the script:

#!/bin/bash

SUCCESS=0
FAIL=0
for i in {1..100}
do
    echo "Running $i"
    go test -timeout 30s -run \
        ^TestLockModeAdvisorySession$ github.com/pressly/goose/v4/tests/e2e/postgres \
        -race -count=1 > /dev/null 2>&1
    case $? in
        0) 
            (( SUCCESS++ ))
            ;;
        1) 
            (( FAIL++ ))
            ;;
        *) 
            echo "Unknown"
            exit 1
            ;;
    esac
done

echo "Success: $SUCCESS Fail: $FAIL"

return retry.RetryableError(err)
}
if !unlocked {
// TODO(mf): provide the user with some documentation on how they can unlock
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added to the documentation as a gotcha, regardless of how edge case this is, when using session-level advisory locks.

options := goose.DefaultOptions().
SetDir(migrationsDir).
SetVerbose(testing.Verbose()).
SetLockMode(goose.LockModeAdvisorySession) /* ---------------- advisory session lock mode */
Copy link
Collaborator Author

@mfridman mfridman Apr 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this satisfies the most common use cases raised in #335 #191 #258 by @tomwhipple @jessedearing @dahu33 and others..

(I still, need to clean up the internal abstractions, but tests are passing and things are shaping up).

I'd also like to add support for transaction-level advisory lock, but this will require a bit more thought on how to handle migrations that MUST be run outside a transaction, see comment by @arvenil.

This kind of falls into "Grouped Migrations" territory #485 #222, where we'll (optionally) attempt to run all-or-nothing migrations if possible. This will require a look ahead on the migrations to be applied, so we can group them based on whether it's safe to be run in a tx or not.

@mfridman mfridman merged commit 620d17b into v4 Apr 2, 2023
7 checks passed
@mfridman mfridman deleted the v4-postgres-lock branch April 2, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant