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.

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/logictest: add logictest for all expected 1PC txn statements #41358

Merged

Conversation

nvanbenschoten
Copy link
Member

First commit from #41324.

We expect a selection of simple single-statement mutations to hit the
"1-phase commit" transaction fast-path. #41320 demonstrated how critical
this is, as regressions here can cause major (> 50%) performance hits to
benchmarks and user workloads. This commit adds a logic test to validate
that these statements continue to hit this fast-path.

Release justification: Testing only.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Have you looked at the tests in opt/exec/execbuilder/testdata/autocommit? I think these belong there

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)

@nvanbenschoten
Copy link
Member Author

Yep, I based some of these tests off that file. #41320 is exactly why that level of testing alone isn't sufficient. In that issue, we saw that autocommit was set by the optimizer but that the deleteRangeNode simply ignored it. We need to test all the way down to the kv batch that is generated for these statements.

I'm happy to move these logic tests to that file though. I'm not sure what the difference between opt/exec/execbuilder/testdata and pkg/sql/logictest/testdata is at this point.

@RaduBerinde
Copy link
Member

I would move them. The logic tests are meant to check that the statements do what they're supposed to. The execbuilder tests are for checking how we run them.

First commit from cockroachdb#41324.

We expect a selection of simple single-statement mutations to hit the
"1-phase commit" transaction fast-path. cockroachdb#41320 demonstrated how critical
this is, as regressions here can cause major (> 50%) performance hits to
benchmarks and user workloads. This commit adds a logic test to validate
that these statements continue to hit this fast-path.

Release justification: Testing only.

Release note: None
@nvanbenschoten
Copy link
Member Author

I would move them. The logic tests are meant to check that the statements do what they're supposed to. The execbuilder tests are for checking how we run them.

Done. PTAL.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

@nvanbenschoten
Copy link
Member Author

bors r+

@nvanbenschoten
Copy link
Member Author

Bors was canceled before it ran.

bors r+

@nvanbenschoten
Copy link
Member Author

bors crash:

{:timeout,
 {GenServer, :call,
  [
    BorsNG.GitHub,
    {:synthesize_commit,
     {{:installation, 117629}, 16563587},
     {%{
        branch: "staging",
        commit_message: "Merge #41358 #41371 #41372 #41379\n\n41358: sql/logictest: add logictest for all expected 1PC txn statements r=nvanbenschoten a=nvanbenschoten\n\nFirst commit from #41324.\r\n\r\nWe expect a selection of simple single-statement mutations to hit the\r\n\"1-phase commit\" transaction fast-path. #41320 demonstrated how critical\r\nthis is, as regressions here can cause major (> 50%) performance hits to\r\nbenchmarks and user workloads. This commit adds a logic test to validate\r\nthat these statements continue to hit this fast-path.\r\n\r\nRelease justification: Testing only.\n\n41371: kv: avoid allocations in client.Txn constructor r=nvanbenschoten a=nvanbenschoten\n\nThis PR contains two small wins that help speed up the client.Txn constructor, which is responsible for **2.90%** of a CPU profile when running Sysbench's `oltp_point_select` workload. The biggest win here will come from addressing https://github.com/cockroachdb/cockroach/issues/32508.\r\n\r\n#### kv: lazily create *RangeIterator in txnPipeliner\r\n\r\nThis allocation was responsible for **0.34%** of a CPU profile when running Sysbench's `oltp_point_select` workload.\r\n\r\n#### kv: only re-alloc refresh spans in augmentMetaLocked if necessary\r\n\r\nThis was responsible for **0.057%** of a CPU profile when running Sysbench's `oltp_point_select` workload.\r\n\r\nRelease justification: None. These can wait for the branch to split.\n\n41372: sql/pgwire: statically allocate format code slice for all text args/cols r=nvanbenschoten a=nvanbenschoten\n\nThis allocation was responsible for **0.8%** of a CPU profile when running\r\nSysbench's oltp_point_select workload.\r\n\r\nRelease justification: Probably none, although this does look very safe.\r\n\r\nRelease note: None\n\n41379: pkg/sql: use ring.Buffer in StmtBuf r=nvanbenschoten a=nvanbenschoten\n\nThe StmtBuf has the exact access patterns typically associated with a ring buffer. Command instances are added to the back of the buffer and trimmed from the front of the buffer. These operations are often performed in lockstep, but that is not always the case, so we need the buffer to be able to grow.\r\n\r\n`ring.Buffer` provides exactly these semantics and it allows us to avoid memory allocations on each Command in `StmtBuf.Push`.\r\n\r\nRelease justification: None. Don't merge until branch is split.\n\nCo-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>\n",
        committer: %{
          email: "bors@cockroachlabs.com",
          name: "craig[bot]"
        },
        parents: ["6d79c7a4f36acd10c410a87ab9bddf3580f28748",
         "a0dde23104734fc2f0ae88397896686433963630",
         "9d682e9c6c112f9246456a1548dc4d3842031257",
         "fdbf14e2395444e68535a30d5be3145750e5b296",
         "42ec21dd7deef1bd5616ea663db0d6b4fe436c91"],
        tree: "6291963028f8f3aeadff552af89a617a6ee464ab"
      }}},
    10000
  ]}}

bors r+

craig bot pushed a commit that referenced this pull request Oct 10, 2019
41356: Revert "opt: disallow mutations under union" r=RaduBerinde a=RaduBerinde

This reverts commit a34d705.

Release justification: recently merge fix no longer needed (thanks
to #41307).

Release note (sql change): Mutations under UNION or UNION ALL are
allowed again.

41358: sql/logictest: add logictest for all expected 1PC txn statements r=nvanbenschoten a=nvanbenschoten

First commit from #41324.

We expect a selection of simple single-statement mutations to hit the
"1-phase commit" transaction fast-path. #41320 demonstrated how critical
this is, as regressions here can cause major (> 50%) performance hits to
benchmarks and user workloads. This commit adds a logic test to validate
that these statements continue to hit this fast-path.

Release justification: Testing only.

41371: kv: avoid allocations in client.Txn constructor r=nvanbenschoten a=nvanbenschoten

This PR contains two small wins that help speed up the client.Txn constructor, which is responsible for **2.90%** of a CPU profile when running Sysbench's `oltp_point_select` workload. The biggest win here will come from addressing #32508.

#### kv: lazily create *RangeIterator in txnPipeliner

This allocation was responsible for **0.34%** of a CPU profile when running Sysbench's `oltp_point_select` workload.

#### kv: only re-alloc refresh spans in augmentMetaLocked if necessary

This was responsible for **0.057%** of a CPU profile when running Sysbench's `oltp_point_select` workload.

Release justification: None. These can wait for the branch to split.

41372: sql/pgwire: statically allocate format code slice for all text args/cols r=nvanbenschoten a=nvanbenschoten

This allocation was responsible for **0.8%** of a CPU profile when running
Sysbench's oltp_point_select workload.

Release justification: Probably none, although this does look very safe.

Release note: None

41379: pkg/sql: use ring.Buffer in StmtBuf r=nvanbenschoten a=nvanbenschoten

The StmtBuf has the exact access patterns typically associated with a ring buffer. Command instances are added to the back of the buffer and trimmed from the front of the buffer. These operations are often performed in lockstep, but that is not always the case, so we need the buffer to be able to grow.

`ring.Buffer` provides exactly these semantics and it allows us to avoid memory allocations on each Command in `StmtBuf.Push`.

Release justification: None. Don't merge until branch is split.

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig
Copy link
Contributor

craig bot commented Oct 10, 2019

Build succeeded

@craig craig bot merged commit a0dde23 into cockroachdb:master Oct 10, 2019
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Oct 10, 2019
Broken by cockroachdb#41358.

The async traces were... async, so the ordering of the trace output
was non-deterministic. This commit fixes the flakiness by filtering
out async trace events.

Release note: None
craig bot pushed a commit that referenced this pull request Oct 11, 2019
41512: sql: fix flake in TestExecBuild/local/autocommit r=nvanbenschoten a=nvanbenschoten

Broken by #41358.

The async traces were... async, so the ordering of the trace output
was non-deterministic. This commit fixes the flakiness by filtering
out async trace events.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/1pcLogicTest branch October 17, 2019 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants