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.

Sign up for GitHub

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

Jump to bottom

workload/tpcc: implement remaining consistency checks #99542

Merged
merged 1 commit into from
Mar 25, 2023
Merged

workload/tpcc: implement remaining consistency checks #99542

merged 1 commit into from
Mar 25, 2023

Conversation

nvanbenschoten
Copy link
Member

This commit adds consistency check 3.3.2.10, 3.3.2.11, and 3.3.2.12 from the TPC-C spec. Incredibly, these have not been touched since cockroachdb/loadgen#85 and cockroachdb/loadgen#165.

We'll be using these consistency checks as part of the testing coverage for upcoming changes to transaction isolation.

Epic: None
Release notes: None

@nvanbenschoten nvanbenschoten requested a review from michae2 March 25, 2023 01:19
@nvanbenschoten nvanbenschoten requested a review from a team as a code owner March 25, 2023 01:19
@nvanbenschoten nvanbenschoten requested review from herkolategan and smg260 and removed request for a team March 25, 2023 01:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/tpccChecks branch from eccec74 to fac3fac Compare March 25, 2023 01:41
michae2
michae2 approved these changes Mar 25, 2023
View reviewed changes
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice! Query plans looked good to me.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @nvanbenschoten, and @smg260)

pkg/workload/tpcc/checks.go line 363 at r1 (raw file):

	return checkNoRows(db, asOfSystemTime, `
SELECT count(*) FROM
  (SELECT c_id,

nit: trailing whitespace

pkg/workload/tpcc/checks.go line 365 at r1 (raw file):

  (SELECT c_id, 
          c_d_id,
          c_w_id,

nit: trailing whitespace

@nvanbenschoten
workload/tpcc: implement remaining consistency checks
58e4295 
This commit adds consistency check 3.3.2.10, 3.3.2.11, and 3.3.2.12
from the TPC-C spec.

We'll be using these consistency checks as part of the testing coverage
for upcoming changes to transaction isolation.

Epic: None
Release notes: None
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/tpccChecks branch from fac3fac to 58e4295 Compare March 25, 2023 17:08
nvanbenschoten
nvanbenschoten commented Mar 25, 2023
View reviewed changes
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r=michae2

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @michae2, and @smg260)

pkg/workload/tpcc/checks.go line 363 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

nit: trailing whitespace

Done.

pkg/workload/tpcc/checks.go line 365 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

nit: trailing whitespace

Done.

@craig
Copy link
Contributor

craig bot commented Mar 25, 2023

Build succeeded:

@craig craig bot merged commit dbd8ae6 into cockroachdb:master Mar 25, 2023
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/tpccChecks branch March 25, 2023 18:59
@renatolabs
Copy link
Contributor

3.3.2.11 has caused a number of roachtest failures (e.g., #98799, #99619, #99594, #99603, #99604).

Are we sure the query is correct?

@nvanbenschoten
Copy link
Member Author

I'm finding that the query is correct, but the condition that it's trying to verify does not and should not hold after running the test. 3.3.2.11 asserts a relationship between the number of rows in the order table and the new_order table. The workload inserts new rows into each of these in the NewOrder transaction. However, it deletes from the new_order table in the Delivery transaction.

Oddly, the spec doesn't mention this explicitly, beyond saying:

Of the twelve conditions, explicit demonstration that the conditions are satisfied is required for the first four only. Demonstration of the last eight consistency conditions is not required because of the lengthy tests which would be necessary.

However, I took a look at what other implementations of TPC-C do with this check and, sure enough, they hit the same issue. For instance, in pingcap/go-tpc#1, they found:

I found that checkCondition11 always failed after running tpcc run command for a while

Long term, we'll want to separate out post-loading checks from post-workload checks. For now, I'll remove the check.

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Mar 27, 2023
@nvanbenschoten
workload/tpcc: disable check 3.3.2.11 after workload
70b1be2 
Fixes cockroachdb#99619.
Fixes cockroachdb#99594.
Fixes cockroachdb#99603.
Fixes cockroachdb#99604.

This commit disables TPC-C's consistency check 3.3.2.11 after the
workload has run. The check asserts a relationship between the number of
rows in the "order" table and rows in the "new_order" table. Rows are
inserted into these tables transactional by the NewOrder transaction.
However, only rows in the "new_order" table are deleted by the Delivery
transaction. Consequently, the consistency condition will fail after the
first Delivery transaction is run by the workload.

See cockroachdb#99542 (comment)
for more details.

Release note: None
@nvanbenschoten nvanbenschoten mentioned this pull request Mar 27, 2023
Merged
craig bot pushed a commit that referenced this pull request Mar 27, 2023
@rickystewart @ericharmeling
@knz @nvanbenschoten
Merge #99509 #99532 #99642 #99660
b1a20ba 
99509: bazci: protect against `nil` target r=healthy-pod a=rickystewart

Closes #98861.

Epic: none

Release note: None

99532: sql: inject stats into TestExecBuild_sql_statistics_persisted testdata r=ericharmeling a=ericharmeling

Background thread: #99240 (comment)

Part of #99399.

This commit replaces the `CREATE STATISTICS` statements in the `TestExecBuild_sql_statistics_persisted` testdata with `ALTER TABLE ... INJECT STATISTICS` and removes the retry directives added to deflake the test in #99447.

Epic: none

Release note: None

99642: serverccl: skip TestServerControllerHTTP under deadlock r=dhartunian a=knz

Fixes #98046.
Release note: None

99660: workload/tpcc: disable check 3.3.2.11 after workload r=renatolabs a=nvanbenschoten

Fixes #99619.
Fixes #99594.
Fixes #99603.
Fixes #99604.
Fixes #99617.
Fixes #99616.
Fixes #99613.
Fixes #99611.
Fixes #99604.
Fixes #99603.
Fixes #99600.
Fixes #99599.
Fixes #99598.
Fixes #99596.
Fixes #99595.
Fixes #99594.

This commit disables TPC-C's consistency check 3.3.2.11 (added in #99542) after the workload has run. The check asserts a relationship between the number of rows in the "order" table and rows in the "new_order" table. Rows are inserted into these tables transactional by the NewOrder transaction. However, only rows in the "new_order" table are deleted by the Delivery transaction. Consequently, the consistency condition will fail after the first Delivery transaction is run by the workload.

See #99542 (comment) for more details.

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
aadityasondhi pushed a commit to aadityasondhi/cockroach that referenced this pull request Mar 28, 2023
@nvanbenschoten @aadityasondhi
workload/tpcc: disable check 3.3.2.11 after workload
ea8209a 
Fixes cockroachdb#99619.
Fixes cockroachdb#99594.
Fixes cockroachdb#99603.
Fixes cockroachdb#99604.

This commit disables TPC-C's consistency check 3.3.2.11 after the
workload has run. The check asserts a relationship between the number of
rows in the "order" table and rows in the "new_order" table. Rows are
inserted into these tables transactional by the NewOrder transaction.
However, only rows in the "new_order" table are deleted by the Delivery
transaction. Consequently, the consistency condition will fail after the
first Delivery transaction is run by the workload.

See cockroachdb#99542 (comment)
for more details.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michae2 michae2 michae2 approved these changes

@herkolategan herkolategan Awaiting requested review from herkolategan herkolategan is a code owner automatically assigned from cockroachdb/test-eng

@smg260 smg260 Awaiting requested review from smg260 smg260 is a code owner automatically assigned from cockroachdb/test-eng

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nvanbenschoten @cockroach-teamcity @renatolabs @michae2