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

sql: fix SCRUB index key order checking #32908

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@lucy-zhang
Copy link
Contributor

lucy-zhang commented Dec 6, 2018

Previously SCRUB would erroneously report that index keys were out of order
for columns other than the first column in an index. This fixes the bug.

Fixes #32874.

Release note (bug fix): Fixed bug where SCRUB would erroneously report that
index keys were out of order.

sql: fix SCRUB index key order checking
Previously `SCRUB` would erroneously report that index keys were out of order
for columns other than the first column in an index. This fixes the bug.

Fixes #32874.

Release note (bug fix): Fixed bug where SCRUB would erroneously report that
index keys were out of order.

@lucy-zhang lucy-zhang requested a review from jordanlewis Dec 6, 2018

@lucy-zhang lucy-zhang requested a review from cockroachdb/sql-rest-prs as a code owner Dec 6, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Dec 6, 2018

This change is Reviewable

@vivekmenezes
Copy link
Contributor

vivekmenezes left a comment

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

}
// After the first column with a differing value is found, the remaining
// columns are skipped (see #32874).
break

This comment has been minimized.

@jordanlewis

jordanlewis Dec 6, 2018

Member

hmm, this doesn't seem quite right to me - won't this miss legitimately out of order columns? I feel like what we need to do is keep a "foundNewValue" bool for each column, setting it to true each time the column has a new value, and only enforce the ordering check for a column that follows a column where foundNewValue is false... does that make sense?

This comment has been minimized.

@jordanlewis

jordanlewis Dec 6, 2018

Member

Nevermind - I got confused. The logic LGTM!

@lucy-zhang

This comment has been minimized.

Copy link
Contributor

lucy-zhang commented Dec 6, 2018

bors r+

craig bot pushed a commit that referenced this pull request Dec 6, 2018

Merge #32908
32908: sql: fix SCRUB index key order checking r=lucy-zhang a=lucy-zhang

Previously `SCRUB` would erroneously report that index keys were out of order
for columns other than the first column in an index. This fixes the bug.

Fixes #32874.

Release note (bug fix): Fixed bug where SCRUB would erroneously report that
index keys were out of order.

Co-authored-by: Lucy Zhang <lucy-zhang@users.noreply.github.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Dec 6, 2018

Build succeeded

@craig craig bot merged commit d5d4009 into cockroachdb:master Dec 6, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

lucy-zhang added a commit to lucy-zhang/cockroach that referenced this pull request Jan 8, 2019

roachtest: add MinVersion to scrub tests
The `scrub` tests have been passing on master, but failing on 2.1 because there
was a bug (fixed in cockroachdb#32908).

Release note: None

craig bot pushed a commit that referenced this pull request Jan 9, 2019

Merge #33573 #33582
33573: roachtest: add MinVersion to scrub tests r=lucy-zhang a=lucy-zhang

The `scrub` tests have been passing on master, but failing on 2.1 because there
was a bug (fixed in #32908).

Release note: None

33582: storage: bump RaftDelaySplitToSuppressSnapshotTicks r=knz a=knz

The admin split path must accommodate a scenario where a range with
not-yet-replicated followers is being manually split multiple
times (eg. IMPORT during TPCC test fixtures). This scenario results in
a bunch of replicas that all need to be populated with
snapshots. To avoid backing up the raft snapshot queue, a heuristic
was put in place (#32594) to delay the admin split if there is another
snapshot being applied already.

As shown by investigation in a failing test, there is a mismatch
between the configured max delay for this heuristic (20s) and the
actual duration of the snapshot application - the latter is limited by
the max bandwidth for snapshots, 2 MB/s resulting in 32s applications
in the worst case. We (Tobias and I) suspect that the heuristic thus
fails to wait long enough to have the protective effect it was
designed to provide.

The current patch increases this delay to exceed this snapshot
application duration estimate to about 50s.

Note that this scenario is not likely to occur now that #32782 has
been merged (this reduces the need for raft snapshots during splits);
however in release-2.1 where that patch was not applied, the scenario
is more likely.

Release note (bug fix): resolve a cluster degradation scenario that
could occur during IMPORT/RESTORE operations, manifested through a
high number of pending Raft snapshots.

Co-authored-by: Lucy Zhang <lucy-zhang@users.noreply.github.com>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment