From bc34337df8170f760bf721d0349f0098ff8a08f1 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Mon, 2 Dec 2019 13:06:48 +0100 Subject: [PATCH 1/2] Makefile: skip ui-test This can be re-enabled after https://github.com/cockroachdb/cockroach/issues/42365 is fixed. Release note: None --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 396399c43464..a478ffc9d93a 100644 --- a/Makefile +++ b/Makefile @@ -1320,7 +1320,7 @@ pkg/ui/dist/%.ccl.dll.js pkg/ui/%.ccl.manifest.json: pkg/ui/webpack.%.js pkg/ui/ .PHONY: ui-test ui-test: $(UI_CCL_DLLS) $(UI_CCL_MANIFESTS) - $(NODE_RUN) -C pkg/ui $(KARMA) start + @echo "would run this but this is skipped pending #42365:" $(NODE_RUN) -C pkg/ui $(KARMA) start .PHONY: ui-test-watch ui-test-watch: $(UI_CCL_DLLS) $(UI_CCL_MANIFESTS) From fad20243f9a9970fc74ca84d753591bed652109c Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Thu, 28 Nov 2019 09:58:00 +0100 Subject: [PATCH 2/2] storage: call EnsureSafeSplitKey during load-based splits We could end up splitting between column families of the same row, which is illegal. Unfortunately the KV layer has to uphold invariants here that it doesn't quite have introspection into, but after this commit it hopefully stops breaking them. See https://github.com/cockroachdb/cockroach/issues/16344 for some additional history. Possibly the solution for #39794. Possibly the solution for #36834. Possibly the solution for #36356. (Intentionally not closing the above; leaving that to the SQL folks). Closes #42056 (which is the go-to for reading up on this issue). Release note (bug fix): prevent a number of panics from the SQL layer caused by an invalid range split. These would usually manifest with messages mentioning encoding errors ("found null on not null column" but also possibly various others). --- pkg/sql/row/fetcher.go | 5 +++++ pkg/storage/split/decider.go | 32 ++++++++++++++++++++++++++- pkg/storage/split/decider_test.go | 36 +++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/pkg/sql/row/fetcher.go b/pkg/sql/row/fetcher.go index 349006aecf17..101e9c2d46f8 100644 --- a/pkg/sql/row/fetcher.go +++ b/pkg/sql/row/fetcher.go @@ -587,6 +587,11 @@ func (rf *Fetcher) NextKey(ctx context.Context) (rowDone bool, err error) { // No more keys in the scan. We need to transition // rf.rowReadyTable to rf.currentTable for the last // row. + // + // NB: this assumes that the KV layer will never split a range + // between column families, which is a brittle assumption. + // See: + // https://github.com/cockroachdb/cockroach/pull/42056 rf.rowReadyTable = rf.currentTable return true, nil } diff --git a/pkg/storage/split/decider.go b/pkg/storage/split/decider.go index 91b13f928625..a13981ab7f03 100644 --- a/pkg/storage/split/decider.go +++ b/pkg/storage/split/decider.go @@ -15,6 +15,7 @@ package split import ( "time" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/util/syncutil" ) @@ -139,7 +140,36 @@ func (d *Decider) MaybeSplitKey(now time.Time) roachpb.Key { d.mu.Lock() d.recordLocked(now, 0, nil) if d.mu.splitFinder != nil && d.mu.splitFinder.Ready(now) { - key = d.mu.splitFinder.Key() + // We've found a key to split at. This key might be in the middle of a + // SQL row. If we fail to rectify that, we'll cause SQL crashes: + // + // https://github.com/cockroachdb/cockroach/pull/42056 + // + // While the behavior at the SQL level is arguably bad and should be + // fixed, splitting between column families is also never a good idea + // for performance in general. So, if the split key is, say + // + // /Table/51/52/53/54/55/9/1 + // + // then we want to split instead at + // + // /Table/51/52/53/54/55 + // + // (see TestDeciderCallsEnsureSafeSplitKey). + // + // The key found here isn't guaranteed to be a valid SQL column family + // key. This is because the keys are sampled from StartKey of requests + // hitting this replica. Ranged operations may well wish to exclude the + // start point by calling .Next() or may span multiple ranges, and so + // such a key may end up being passed to EnsureSafeSplitKey here. + // + // We take the risk that the result may sometimes not be a good split + // point (or even in this range). + var err error + key, err = keys.EnsureSafeSplitKey(d.mu.splitFinder.Key()) + if err != nil { + key = nil + } } d.mu.Unlock() diff --git a/pkg/storage/split/decider_test.go b/pkg/storage/split/decider_test.go index 3cc03848e073..b023d5e843f8 100644 --- a/pkg/storage/split/decider_test.go +++ b/pkg/storage/split/decider_test.go @@ -15,9 +15,12 @@ import ( "testing" "time" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDecider(t *testing.T) { @@ -170,3 +173,36 @@ func TestDecider(t *testing.T) { assert.Nil(t, d.MaybeSplitKey(ms(tick))) assert.Nil(t, d.mu.splitFinder) } + +func TestDeciderCallsEnsureSafeSplitKey(t *testing.T) { + defer leaktest.AfterTest(t)() + intn := rand.New(rand.NewSource(11)).Intn + + var d Decider + Init(&d, intn, func() float64 { return 1.0 }) + + baseKey := keys.MakeTablePrefix(51) + for i := 0; i < 4; i++ { + baseKey = encoding.EncodeUvarintAscending(baseKey, uint64(52+i)) + } + c0 := func() roachpb.Span { return roachpb.Span{Key: append([]byte(nil), keys.MakeFamilyKey(baseKey, 1)...)} } + c1 := func() roachpb.Span { return roachpb.Span{Key: append([]byte(nil), keys.MakeFamilyKey(baseKey, 9)...)} } + + expK, err := keys.EnsureSafeSplitKey(c1().Key) + require.NoError(t, err) + + var k roachpb.Key + var now time.Time + for i := 0; i < 2*int(minSplitSuggestionInterval/time.Second); i++ { + now = now.Add(500 * time.Millisecond) + d.Record(now, 1, c0) + now = now.Add(500 * time.Millisecond) + d.Record(now, 1, c1) + k = d.MaybeSplitKey(now) + if len(k) != 0 { + break + } + } + + require.Equal(t, expK, k) +}