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: 23.1 microbenchmark regression investigation and sign-off #98306

Closed
11 of 18 tasks
mgartner opened this issue Mar 9, 2023 · 5 comments
Closed
11 of 18 tasks

sql: 23.1 microbenchmark regression investigation and sign-off #98306

mgartner opened this issue Mar 9, 2023 · 5 comments
Labels
branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Mar 9, 2023

This issue tracks SQL Queries -related benchmark regressions. The main issue tracking all benchmark regressions is #96960.

The pkg/sql sheet is here: https://docs.google.com/spreadsheets/d/1uDYotBARIqdL5nSquMoF7Fy3qqoMfNigwHAcB8NPR7M/edit?pli=1#gid=7

Here's a list of the major regressions we need to investigate:

time/op regressions (up to 20%)

alloc/op regressions

TODO

allocs/op regressions

TODO

@mgartner mgartner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Mar 9, 2023
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Mar 9, 2023
@mgartner mgartner added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. GA-blocker labels Mar 9, 2023
@blathers-crl
Copy link

blathers-crl bot commented Mar 9, 2023

Hi @mgartner, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner mgartner removed the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Mar 9, 2023
@blathers-crl
Copy link

blathers-crl bot commented Mar 9, 2023

Hi @mgartner, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner mgartner added the branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 label Mar 9, 2023
@mgartner
Copy link
Collaborator Author

mgartner commented Mar 9, 2023

We might need to investigate some regressions in pkg/ccl too: https://docs.google.com/spreadsheets/d/1HxTZbtEMscgzt_4f-z1D2KB6HdvXw-qGaZF4zkXV1ik/edit#gid=6

@mgartner
Copy link
Collaborator Author

mgartner commented Mar 10, 2023

Here is a comparison of the optimizer benchmarks comparing release-22.2 to master after making the join reordering limit consistent: https://gist.github.com/mgartner/9e2bff7eaef825d0224363b41538c973

All the regressions seem acceptable. The only minor ones that we might want to look into are some of the bytes allocated regressions:

Phases/many-columns-and-indexes-b/Simple/ExecBuild-24                      26.7kB ± 0%    48.7kB ± 0%   +82.15%  (p=0.008 n=5+5)
Phases/many-columns-and-indexes-b/Simple/Explore-24                        25.7kB ± 0%    47.5kB ± 0%   +85.35%  (p=0.008 n=5+5)
Phases/many-columns-and-indexes-b/Simple/OptBuildNorm-24                   24.2kB ± 0%    46.1kB ± 0%   +90.67%  (p=0.008 n=5+5)
Phases/many-columns-and-indexes-b/Simple/OptBuildNoNorm-24                 20.4kB ± 0%    42.2kB ± 0%  +106.99%  (p=0.008 n=5+5)
Phases/many-columns-and-indexes-a/Simple/ExecBuild-24                      18.6kB ± 0%    40.0kB ± 0%  +115.37%  (p=0.016 n=4+5)
Phases/many-columns-and-indexes-a/Simple/Explore-24                        17.8kB ± 0%    39.2kB ± 0%  +119.77%  (p=0.008 n=5+5)
Phases/many-columns-and-indexes-a/Simple/OptBuildNorm-24                   16.4kB ± 0%    37.8kB ± 0%  +130.76%  (p=0.008 n=5+5)
Phases/many-columns-and-indexes-a/Simple/OptBuildNoNorm-24                 14.2kB ± 0%    35.6kB ± 0%  +149.72%  (p=0.016 n=4+5)

mgartner added a commit to mgartner/cockroach that referenced this issue Mar 15, 2023
This commit fixes a performance regression in pgwire encoding of tuples
introduced in cockroachdb#95009.

Informs cockroachdb#98306

Epic: None

Release note: None
craig bot pushed a commit that referenced this issue Mar 16, 2023
98682: roachprod: provide workaround for long-running AWS clusters r=srosenberg a=renatolabs

In #98076, we started validating hostnames before running any commands to avoid situations where a stale cache could lead to unintended interference with other clusters due to public IP reuse. The check relies on the VM's `hostname` matching the expected cluster name in the cache. GCP and Azure clusters set the hostname to the instance name by default, but that is not the case for AWS; the aforementioned PR explicitly sets the hostname when the instance is created.

However, in the case of long running AWS clusters (created before host validation was introduced) or clusters that are created with an outdated version of `roachprod`, the hostname will still be the default AWS hostname, and any interaction with that cluster will fail if using a recent `roachprod` version. To remedy this situation, this commit includes:

* better error reporting. When we attempt to run a command on an AWS cluster and host validation fails, we display a message to the user explaining that their hostnames may need fixing.

* if the user confirms that the cluster still exists (by running `roachprod list`), they are able to automatically fix the hostnames to the expected value by running a new `fix-long-running-aws-hostnames` command. This is a temporary workaround that should be removed once we no longer have clusters that would be affected by this issue.

This commit will be reverted once we no longer have clusters created with the default hostnames; this will be easier to achieve once we have an easy way for everyone to upgrade their `roachprod` (see #97311).

Epic: none

Release note: None

98717: tree: fix tuple encoding performance regression r=mgartner a=mgartner

This commit fixes a performance regression in pgwire encoding of tuples
introduced in #95009.

Informs #98306

Epic: None

Release note: None

Co-authored-by: Renato Costa <renato@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this issue Mar 16, 2023
This commit fixes a performance regression in pgwire encoding of tuples
introduced in #95009.

Informs #98306

Epic: None

Release note: None
blathers-crl bot pushed a commit that referenced this issue Mar 16, 2023
This commit fixes a performance regression in pgwire encoding of tuples
introduced in #95009.

Informs #98306

Epic: None

Release note: None
@mgartner
Copy link
Collaborator Author

We've accounted for the major regressions. I'm going to close this. Thanks for the help @yuzefovich and @DrewKimball!

@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker T-sql-queries SQL Queries Team
Projects
Archived in project
Development

No branches or pull requests

1 participant