-
-
Notifications
You must be signed in to change notification settings - Fork 507
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #6744 from dolthub/aaron/fix-hasCache-dangling-ref…
…erences-bug-2 go/store/nbs: store.go: Clean up how we update hasCache so that we only update it after successfully writing the memtable.
- Loading branch information
Showing
4 changed files
with
165 additions
and
83 deletions.
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ package doltdb_test | |
|
||
import ( | ||
"context" | ||
"errors" | ||
"os" | ||
"testing" | ||
|
||
"github.com/dolthub/go-mysql-server/sql" | ||
|
@@ -28,7 +30,13 @@ import ( | |
"github.com/dolthub/dolt/go/libraries/doltcore/env" | ||
"github.com/dolthub/dolt/go/libraries/doltcore/ref" | ||
"github.com/dolthub/dolt/go/libraries/doltcore/sqle" | ||
"github.com/dolthub/dolt/go/libraries/utils/filesys" | ||
"github.com/dolthub/dolt/go/store/hash" | ||
"github.com/dolthub/dolt/go/store/nbs" | ||
"github.com/dolthub/dolt/go/store/prolly" | ||
"github.com/dolthub/dolt/go/store/prolly/tree" | ||
"github.com/dolthub/dolt/go/store/types" | ||
"github.com/dolthub/dolt/go/store/val" | ||
) | ||
|
||
func TestGarbageCollection(t *testing.T) { | ||
|
@@ -40,6 +48,8 @@ func TestGarbageCollection(t *testing.T) { | |
testGarbageCollection(t, gct) | ||
}) | ||
} | ||
|
||
t.Run("HasCacheDataCorruption", testGarbageCollectionHasCacheDataCorruptionBugFix) | ||
} | ||
|
||
type stage struct { | ||
|
@@ -140,3 +150,118 @@ func testGarbageCollection(t *testing.T, test gcTest) { | |
require.NoError(t, err) | ||
assert.Equal(t, test.expected, actual) | ||
} | ||
|
||
// In September 2023, we found a failure to handle the `hasCache` in | ||
// `*NomsBlockStore` appropriately while cleaning up a memtable into which | ||
// dangling references had been written could result in writing chunks to a | ||
// database which referenced non-existant chunks. | ||
// | ||
// The general pattern was to get new chunk addresses into the hasCache, but | ||
// not written to the store, and then to have an incoming chunk add a refenece | ||
// to missing chunk. At that time, we would clear the memtable, since it had | ||
// invalid chunks in it, but we wouldn't purge the hasCache. Later writes which | ||
// attempted to reference the chunks which had made it into the hasCache would | ||
// succeed. | ||
// | ||
// One such concrete pattern for doing this is implemented below. We do: | ||
// | ||
// 1) Put a new chunk to the database -- C1. | ||
// | ||
// 2) Run a GC. | ||
// | ||
// 3) Put a new chunk to the database -- C2. | ||
// | ||
// 4) Call NBS.Commit() with a stale last hash.Hash. This causes us to cache C2 | ||
// as present in the store, but it does not get written to disk, because the | ||
// optimistic concurrency control on the value of the current root hash fails. | ||
// | ||
// 5) Put a chunk referencing C1 to the database -- R1. | ||
// | ||
// 5) Call NBS.Commit(). This causes ErrDanglingRef. C1 was written before the | ||
// GC and is no longer in the store. C2 is also cleared from the pending write | ||
// set. | ||
// | ||
// 6) Put a chunk referencing C2 to the database -- R2. | ||
// | ||
// 7) Call NBS.Commit(). This should fail, since R2 references C2 and C2 is not | ||
// in the store. However, C2 is in the cache as a result of step #4, and so | ||
// this does not fail. R2 gets written to disk with a dangling reference to C2. | ||
func testGarbageCollectionHasCacheDataCorruptionBugFix(t *testing.T) { | ||
ctx := context.Background() | ||
|
||
d, err := os.MkdirTemp(t.TempDir(), "hascachetest-") | ||
require.NoError(t, err) | ||
|
||
ddb, err := doltdb.LoadDoltDB(ctx, types.Format_DOLT, "file://"+d, filesys.LocalFS) | ||
require.NoError(t, err) | ||
defer ddb.Close() | ||
|
||
err = ddb.WriteEmptyRepo(ctx, "main", "Aaron Son", "[email protected]") | ||
require.NoError(t, err) | ||
|
||
root, err := ddb.NomsRoot(ctx) | ||
require.NoError(t, err) | ||
|
||
ns := ddb.NodeStore() | ||
|
||
c1 := newIntMap(t, ctx, ns, 1, 1) | ||
_, err = ns.Write(ctx, c1.Node()) | ||
require.NoError(t, err) | ||
|
||
err = ddb.GC(ctx, nil) | ||
require.NoError(t, err) | ||
|
||
c2 := newIntMap(t, ctx, ns, 2, 2) | ||
_, err = ns.Write(ctx, c2.Node()) | ||
require.NoError(t, err) | ||
|
||
success, err := ddb.CommitRoot(ctx, c2.HashOf(), c2.HashOf()) | ||
require.NoError(t, err) | ||
require.False(t, success, "committing the root with a last hash which does not match the current root must fail") | ||
|
||
r1 := newAddrMap(t, ctx, ns, "r1", c1.HashOf()) | ||
_, err = ns.Write(ctx, r1.Node()) | ||
require.NoError(t, err) | ||
|
||
success, err = ddb.CommitRoot(ctx, root, root) | ||
require.True(t, errors.Is(err, nbs.ErrDanglingRef), "committing a reference to just-collected c1 must fail with ErrDanglingRef") | ||
|
||
r2 := newAddrMap(t, ctx, ns, "r2", c2.HashOf()) | ||
_, err = ns.Write(ctx, r2.Node()) | ||
require.NoError(t, err) | ||
|
||
success, err = ddb.CommitRoot(ctx, root, root) | ||
require.True(t, errors.Is(err, nbs.ErrDanglingRef), "committing a reference to c2, which was erased with the ErrDanglingRef above, must also fail with ErrDanglingRef") | ||
} | ||
|
||
func newIntMap(t *testing.T, ctx context.Context, ns tree.NodeStore, k, v int8) prolly.Map { | ||
desc := val.NewTupleDescriptor(val.Type{ | ||
Enc: val.Int8Enc, | ||
Nullable: false, | ||
}) | ||
|
||
tb := val.NewTupleBuilder(desc) | ||
tb.PutInt8(0, k) | ||
keyTuple := tb.Build(ns.Pool()) | ||
|
||
tb.PutInt8(0, v) | ||
valueTuple := tb.Build(ns.Pool()) | ||
|
||
m, err := prolly.NewMapFromTuples(ctx, ns, desc, desc, keyTuple, valueTuple) | ||
require.NoError(t, err) | ||
return m | ||
} | ||
|
||
func newAddrMap(t *testing.T, ctx context.Context, ns tree.NodeStore, key string, h hash.Hash) prolly.AddressMap { | ||
m, err := prolly.NewEmptyAddressMap(ns) | ||
require.NoError(t, err) | ||
|
||
editor := m.Editor() | ||
err = editor.Add(ctx, key, h) | ||
require.NoError(t, err) | ||
|
||
m, err = editor.Flush(ctx) | ||
require.NoError(t, err) | ||
|
||
return m | ||
} |
This file contains 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
This file contains 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
This file contains 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