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

storage: enable EFOS, excise on snapshot, ingest splitting performance regressions #117494

Closed
mgartner opened this issue Jan 8, 2024 · 5 comments · Fixed by #117542
Closed

storage: enable EFOS, excise on snapshot, ingest splitting performance regressions #117494

mgartner opened this issue Jan 8, 2024 · 5 comments · Fixed by #117542
Assignees
@mgartner
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-23.2.0-rc (deleted) C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-storage Storage Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Jan 8, 2024 •
edited
Loading

See https://cockroachlabs.slack.com/archives/C05SHB3GAGP/p1704727315971649?thread_ts=1704720001.230629&cid=C05SHB3GAGP

And https://docs.google.com/spreadsheets/d/1G6JyuumetcpfRB2thIkXtTgYZCVeXShYPdbncwoLRM8/edit#gid=6

I've tracked all the regressions below to #117116.

Regressions

BenchmarkQueryCache

image

BenchmarkFKInsert

image

BenchmarkResolveFunction

image

BenchmarkSequenceIncrement

image

Jira issue: CRDB-35215
@mgartner mgartner added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-23.2.0-rc (deleted) blocks-23.2.0-rc.1 labels Jan 8, 2024
@mgartner
Copy link
Collaborator Author

mgartner commented Jan 8, 2024

Let's first determine if these regressions are present on the release-23.2 and RC branches, or master only.

@mgartner
Copy link
Collaborator Author

mgartner commented Jan 8, 2024

cc @cockroachdb/storage for awareness of the large BenchmarkSequenceIncrement regression due to 945b21b.

@blathers-crl blathers-crl bot added the T-storage Storage Team label Jan 8, 2024
@RaduBerinde RaduBerinde mentioned this issue Jan 8, 2024
Merged
@mgartner mgartner changed the title sql: investigate recent benchmark regressions storage: enable EFOS, excise on snapshot, ingest splitting performance regressions Jan 8, 2024
@mgartner mgartner removed release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. blocks-23.2.0-rc.1 labels Jan 8, 2024
@mgartner
Copy link
Collaborator Author

mgartner commented Jan 8, 2024

I've removed the release-blocker label because the settings that caused these regressions are disabled by default on 23.2.

@mgartner mgartner self-assigned this Jan 9, 2024
@jbowens
Copy link
Collaborator

jbowens commented Jan 9, 2024

@mgartner Are you cool with queries taking this back as a task to update relevant benchmarks to not time TestCluster shutdown?

@mgartner
Copy link
Collaborator Author

mgartner commented Jan 9, 2024

Yep, already on it.

mgartner added a commit to mgartner/cockroach that referenced this issue Jan 9, 2024
@mgartner
sql: fix erroneous benchmark regressions
9deee4e 
Several benchmarks incorrectly included cluster shutdown as part of the
benchmark timing. This caused major regressions in benchmarks
when cockroachdb#117116 was merged because it made cluster shutdown slower. The
benchmarks now stop the timer before initiating cluster shutdown to more
accurately measure the code in question.

Fixes cockroachdb#117494

Release note: None
@mgartner mgartner mentioned this issue Jan 9, 2024
Merged
craig bot pushed a commit that referenced this issue Jan 11, 2024
@andrewbaptist @mgartner
@homa31 @Xiang-Gu
Merge #113893 #117542 #117558 #117569 #117572
f428c19 
113893: cli: remove cockroach connect r=nvanbenschoten a=andrewbaptist

`connect` was implemented as an experiment to allow bootstrapping nodes from other nodes CA's (#60632). The details are described here: https://github.com/aaron-crl/toy-secure-init-handshake/tree/n-way-join

This implementation was never completed, and the visibility of this code can cause confusion. This PR removes all the code with the idea that we can bring it back later if necessary.

Epic: none

Release note (cli change): Removal of the `cockroach connect` functionality.

117542: sql: fix erroneous benchmark regressions r=mgartner a=mgartner

Several benchmarks incorrectly included cluster shutdown as part of the
benchmark timing. This caused major regressions in benchmarks
when #117116 was merged because it made cluster shutdown slower. The
benchmarks now stop the timer before initiating cluster shutdown to more
accurately measure the code in question.

Fixes #117494

Release note: None


117558: sql: add telemetry block regexps and fix flake r=mgartner a=mgartner

#### sqltestutils: allow blocking regexps in telemetry feature list

The `feature-allowlist` directive for telemetry tests has been renamed
`feature-list` and it now supports blocking regexps. Any line in the
`feature-list` that starts with "!" is a block regexp that prevents any
features matching the regexp from being included in the output of the
`feature-usage` and `feature-counters` directives. This is helpful
because Go's regexps do not support negative look-aheads, so adding
features that should not be matched is difficult and tedious.

Release note: None

#### sql: ignore "sql.schema.create_stats" in schema telemetry test

This commit ignores the `sql.schema.create_stats` in the `schema`
telemetry tests because it causes sporadic failures.

Fixes #117309

Release note: None


117569: authors: add homa31 to authors r=homa31 a=homa31

Release note: None
Epic: None

117572: *: Prep work for supporting CONFIGURE ZONE in declarative schema changer r=Xiang-Gu a=Xiang-Gu

This PR consists several preparation work that will be needed for properly supporting CONFIGURE ZONE stmts in declarative schema changer. They're separated out for easier review and bc I expect those to be a lot less controversial than the main work. See each commit for details.

Informs #117574
Epic: CRDB-31473

Co-authored-by: Andrew Baptist <baptist@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Howard Ma <howard.ma@cockroachlabs.com>
Co-authored-by: Xiang Gu <xiang@cockroachlabs.com>
@craig craig bot closed this as completed in 5e77ef4 Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mgartner mgartner

Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-23.2.0-rc (deleted) C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-storage Storage Team
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: fix erroneous benchmark regressions mgartner/cockroach
2 participants
@jbowens @mgartner