Skip to content

storage/engine: don't decrement IntentCount on system intent abort#39427

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixMetricsFlake
Aug 8, 2019
Merged

storage/engine: don't decrement IntentCount on system intent abort#39427
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixMetricsFlake

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Aug 7, 2019

Fixes #39390.
Fixes #39407.
Fixes #39370.
Fixes #39419.

A bug introduced in #39251 was causing the IntentCount field in MVCCStats to get out of sync.

I was expecting TestMVCCStatsRandomized/sys/inline=false to easily catch the bug, but it wasn't. It turns out that the test was updating the transaction's write timestamp but not its read timestamp, which was causing it to hit this error:

return errors.Errorf("mvccPutInternal: txn's read timestamp %s does not match timestamp %s",

This was preventing the randomized test from performing the series of steps we needed it to hit the bug.

This commit fixes the bug, fixes the randomized test so that it would have caught it, and adds a targeted regression test for this case.

Release note: None

Fixes cockroachdb#39390.
Fixes cockroachdb#39407.
Fixes cockroachdb#39370.
Fixes cockroachdb#39419.

A bug introduced in cockroachdb#39251 was causing the IntentCount field in MVCCStats
to get out of sync.

I was expecting `TestMVCCStatsRandomized/sys/inline=false` to easily
catch the bug, but it wasn't. It turns out that the test was updating
the transaction's write timestamp but not its read timestamp, which
was causing it to hit this error:
https://github.com/cockroachdb/cockroach/blob/41cb28e85575292926790e79912c893d13e1054d/pkg/storage/engine/mvcc.go#L1349
This was preventing the randomized test from performing the series of steps
we needed it to to hit the bug.

This commit fixes the bug, fixes the randomized test so that it would
have caught it, and adds a targeted regression test for this case.

Release note: None
@nvb nvb requested a review from dt August 7, 2019 22:10
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Aug 8, 2019

bors r+

craig bot pushed a commit that referenced this pull request Aug 8, 2019
39206: sql: add RevertTable wrapper for RevertRangeRequest r=dt a=dt

(first commit is #39251)

kv,batcheval: fix RevertRange to work with DistSender and ResumeSpans

    This patch includes several changes required to make RevertRange requests
    get along with DistSender:

    1) add RevertRange to the explicit check for maxkey-supported requests.
    2) makes RevertRangeResponse combinable, however combining assumes
    that at-most-one reply returned a ResumeSpan, so it also
    3) sets NumKeys to MaxKeys in RevertRange when returning a ResumeSpan,
    to stop distsender from sending any further requests that might also hit
    a resume and then trip up combining.

    Together these make it possible to send batches of RevertRange requests
    via DistSender and get back resume spans.

    Release note: none.

sql: add RevertTables helper for reverting a Table via RevertRange

    This adds a helper that can revert a collection of tables to a target
    timestamp using the RevertRange RPC.

    It checks that the tables do not  overlap the key space of another table
    not being reverted (i.e. that all the reverting tables are interleaved,
    if at all, only with each other) and that the tables are offline.

    It then reverts the table spans using RevertRange, including handling
    the ResumeSpan pagination.

    Release note: none.

39427: storage/engine: don't decrement IntentCount on system intent abort r=nvanbenschoten a=nvanbenschoten

Fixes #39390.
Fixes #39407.
Fixes #39370.
Fixes #39419.

A bug introduced in #39251 was causing the IntentCount field in MVCCStats to get out of sync.

I was expecting `TestMVCCStatsRandomized/sys/inline=false` to easily catch the bug, but it wasn't. It turns out that the test was updating the transaction's write timestamp but not its read timestamp, which was causing it to hit this error:
https://github.com/cockroachdb/cockroach/blob/41cb28e85575292926790e79912c893d13e1054d/pkg/storage/engine/mvcc.go#L1349
This was preventing the randomized test from performing the series of steps we needed it to hit the bug.

This commit fixes the bug, fixes the randomized test so that it would have caught it, and adds a targeted regression test for this case.

Release note: None

Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 8, 2019

Build succeeded

@craig craig bot merged commit 2edae4f into cockroachdb:master Aug 8, 2019
@nvb nvb deleted the nvanbenschoten/fixMetricsFlake branch August 9, 2019 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants