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 for concurrency race #84

Merged
merged 2 commits into from
Jun 7, 2022
Merged

Fix for concurrency race #84

merged 2 commits into from
Jun 7, 2022

Conversation

thorfour
Copy link
Contributor

@thorfour thorfour commented Jun 4, 2022

If we start a new transaction, reads that started after the last
committed write, but before the compaction wont be able to find those commited
reads. Instead copy the watermark as the tx id to allow those reads to
be able to read all the committed data.

This is should be the fix for #71

@thorfour thorfour requested a review from brancz June 4, 2022 02:55
@thorfour thorfour marked this pull request as ready for review June 4, 2022 03:00
@thorfour
Copy link
Contributor Author

thorfour commented Jun 4, 2022

hmm I think this caused a different problem where we now read more data Fixed by #85

--- FAIL: Test_Table_Concurrency (4.62s)
    --- FAIL: Test_Table_Concurrency/1024 (1.10s)
        table_test.go:33: level=error msg="failed to delete granule during split"

        table_test.go:33: level=debug msg="read blocks" n=0

        table_test.go:523:
            	Error Trace:	table_test.go:523
            	Error:      	Not equal:
            	            	expected: 8000
            	            	actual  : 9663
            	Test:       	Test_Table_Concurrency/1024
            	```

If we start a new transaction, reads that started after the last
committed write, but before the compaction wont be able to find those commited
reads. Instead copy the watermark as the tx id to allow those reads to
be able to read all the committed data.
@thorfour
Copy link
Contributor Author

thorfour commented Jun 4, 2022

With the latest fix from @ostafen and this fix I'm no longer seeing any failures with the concurrency test.

@MadhavJivrajani
Copy link
Contributor

MadhavJivrajani commented Jun 5, 2022

Not sure if I'm missing something but I tried running the test again with the race detector enabled and I still get races detected:

gh pr checkout 84

HEAD is at a3e6195e7beef1e77eff933d569555eb474772f7

go test -race -run Test_Table_Concurrency

The logs can be found here: https://gist.github.com/MadhavJivrajani/fa69278be76cdf18143dcde533f0e193

Edit: This is probably unrelated to the bug being fixed by the PR tho

@thorfour
Copy link
Contributor Author

thorfour commented Jun 5, 2022

Not sure if I'm missing something but I tried running the test again with the race detector enabled and I still get races detected:

gh pr checkout 84

HEAD is at a3e6195e7beef1e77eff933d569555eb474772f7

go test -race -run Test_Table_Concurrency

The logs can be found here: https://gist.github.com/MadhavJivrajani/fa69278be76cdf18143dcde533f0e193

Edit: This is probably unrelated to the bug being fixed by the PR tho

This is a new one, but definitely unrelated to this fix. Can you open a second issue with this information?

@MadhavJivrajani
Copy link
Contributor

Created #86

@@ -490,8 +490,8 @@ func (t *TableBlock) splitGranule(granule *Granule) {
return
}

// Obtain a new tx for this compaction
tx, watermark, commit := t.table.db.begin()
// Use the latest watermark as the tx id
Copy link
Member

Choose a reason for hiding this comment

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

This is safe to do because we're not actually adding new data, we're just compacting it, right?

Copy link
Contributor Author

@thorfour thorfour Jun 7, 2022

Choose a reason for hiding this comment

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

Correct

@brancz brancz merged commit 19ee115 into main Jun 7, 2022
@brancz brancz deleted the fix-compact-race branch June 7, 2022 07:56
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.

3 participants