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

release-23.1: ttl: use better defaults for some session variables #118496

Merged

Conversation

yuzefovich
Copy link
Member

Backport 1/2 commits from #118330.

/cc @cockroachdb/release


This commit fixes some problems that we've seen around internally-executed queries issued by the TTL jobs, present before 23.2. In particular, on 23.1 and prior releases we used Go defaults for the session data parameters for internally-executed queries, which can differ from the defaults set by CRDB. As a result, query plans could be suboptimal as we've seen in a couple of recent escalations. This bug has been fixed on 23.2 proper (we now use CRDB defaults for all session variables except for one), and on 23.1 and prior this commit applies a targeted fix only to the TTL queries. In particular, the following overrides are now set:

  • reorder_joins_limit to the default value of 8
  • optimizer_use_histograms and optimizer_use_multi_col_stats are both set to true.

On 23.2 and later only optimizer_use_histograms needs to be updated since it's the only exception mentioned above. However, I chose to keep all 3 in the same commit so that it can be backported more easily to 23.1, and the following commit will revert the other 2 on 23.2 and later.

Touches: #102954.
Fixes: #118129.

Release note (bug fix): Internal queries issued by the TTL jobs should now use optimal plans. The bug has been present since at least 22.2 version.

Release justification: bug fix.

This commit fixes some problems that we've seen around
internally-executed queries issued by the TTL jobs, present before 23.2.
In particular, on 23.1 and prior releases we used Go defaults for the
session data parameters for internally-executed queries, which can
differ from the defaults set by CRDB. As a result, query plans could be
suboptimal as we've seen in a couple of recent escalations. This bug has
been fixed on 23.2 proper (we now use CRDB defaults for all session
variables except for one), and on 23.1 and prior this commit applies
a targeted fix only to the TTL queries. In particular, the following
overrides are now set:
- `reorder_joins_limit` to the default value of 8
- `optimizer_use_histograms` and `optimizer_use_multi_col_stats` are
both set to `true`.

On 23.2 and later only `optimizer_use_histograms` needs to be updated
since it's the only exception mentioned above. However, I chose to keep
all 3 in the same commit so that it can be backported more easily to
23.1, and the following commit will revert the other 2 on 23.2 and
later.

Release note (bug fix): Internal queries issued by the TTL jobs should
now use optimal plans. The bug has been present since at least 22.2
version.
@yuzefovich yuzefovich requested a review from a team as a code owner January 30, 2024 19:30
Copy link

blathers-crl bot commented Jan 30, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL and one additional
    TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Jan 30, 2024
Copy link

blathers-crl bot commented Jan 30, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@yuzefovich yuzefovich merged commit 0fea8da into cockroachdb:release-23.1 Feb 1, 2024
6 checks passed
@yuzefovich yuzefovich deleted the backport23.1-118330 branch February 1, 2024 00:01
@yuzefovich
Copy link
Member Author

blathers backport release-23.1.15-rc

Copy link

blathers-crl bot commented Feb 1, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error setting reviewers, but backport branch blathers/backport-release-23.1.15-rc-118496 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/118564/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch release-23.1.15-rc failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants