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

fix: race condition in both upsert post and upsert patch #88

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

luthfifahlevi
Copy link

@luthfifahlevi luthfifahlevi commented Feb 7, 2025

When some jobs that has same recipe run at the same time, it makes those assets has the version in assets_versions table greater than version in assets table. This is not expected because version in assets table should be the (new) highest number, so should be equals to highest number of the version in assets_versions . The expectation is with transaction sql (the race condition should be handled), it will insert new rows in assets_version with increasing minor version (exp: 0.9 → 0.10) IF ONLY there is an update on the job AND has the changelog.

For example bug case:

  • asset_test_a in assets table has version 0.99
  • The highest version for asset_test_a in assets_versions table has version 0.100
  • When update the assets and has change log, it will raise error duplicate key value violates unique constraint "assets_versions_idx_asset_id_version" since combination asset_test_a ID and 0.100 already exist

The solution:
Using Row-level Locks (ref: https://www.postgresql.org/docs/current/explicit-locking.html#LOCKING-ROWS) with SELECT FOR UPDATE within same transaction for update

@luthfifahlevi luthfifahlevi marked this pull request as draft February 7, 2025 08:13
@coveralls
Copy link

coveralls commented Feb 8, 2025

Pull Request Test Coverage Report for Build 13215384412

Details

  • 53 of 71 (74.65%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 83.983%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/store/postgres/asset_repository.go 53 71 74.65%
Files with Coverage Reduction New Missed Lines %
internal/store/postgres/asset_repository.go 1 78.03%
Totals Coverage Status
Change from base Build 12114630025: -0.03%
Covered Lines: 6890
Relevant Lines: 8204

💛 - Coveralls

@luthfifahlevi luthfifahlevi marked this pull request as ready for review February 8, 2025 11:59
@luthfifahlevi luthfifahlevi self-assigned this Feb 8, 2025
@@ -598,12 +610,15 @@ func (r *AssetRepository) insert(ctx context.Context, ast *asset.Asset) (string,
return id, nil
}

func (r *AssetRepository) update(ctx context.Context, assetID string, newAsset, oldAsset *asset.Asset, clog diff.Changelog) error {
func (r *AssetRepository) update(ctx context.Context, tx *sqlx.Tx, newAsset, oldAsset *asset.Asset, clog diff.Changelog) error {
assetID := oldAsset.ID
Copy link

Choose a reason for hiding this comment

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

is there any case where the assetID changed?

Copy link
Author

@luthfifahlevi luthfifahlevi Feb 10, 2025

Choose a reason for hiding this comment

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

should be not. Its UUID anyway, so ideally we use URN instead for Upsert identifier

And actually assetID came from fetchedAsset

err = r.update(ctx, fetchedAsset.ID, ast, &fetchedAsset, changelog)
which is it got from existing asset (old asset) by GetByURN, so I just refactor it to remove redundancy

@@ -176,6 +180,10 @@ func (r *AssetRepository) GetByURN(ctx context.Context, urn string) (asset.Asset
}

func (r *AssetRepository) getWithPredicate(ctx context.Context, pred sq.Eq) (asset.Asset, error) {
Copy link
Member

Choose a reason for hiding this comment

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

we can remove this function if not needed

Copy link
Author

@luthfifahlevi luthfifahlevi Feb 10, 2025

Choose a reason for hiding this comment

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

still used in GetAssetByID API here:

ast, err = s.assetRepository.GetByID(ctx, id)

@luthfifahlevi luthfifahlevi changed the title fix: upsert race condition fix: race condition in both upsert post and upsert patch Feb 11, 2025
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.

4 participants