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: fix logic to collect stats on system.jobs #108139

Merged
merged 1 commit into from
Aug 5, 2023

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Aug 3, 2023

This commit fixes an oversight in #102637 which intended to enable stats collection on the jobs table, but was not successful.

I've manually confirmed that stats are now collected on the jobs table in a local cluster:

  888074673664065537 | AUTO CREATE STATS               | Table statistics refresh for system.public.jobs                         | CREATE STATISTICS __auto__ FROM [15] WITH OPTIONS THROTTLING 0.9 AS OF SYSTEM TIME '-30s'  | root      | succeeded | NULL           | 2023-08-03 19:01:22.343

Informs #107405

Release note (performance improvement): We now automatically collect table statistics on the system.jobs table, which will enable the optimizer to produce better query plans for internal queries that access the system.jobs table. This may result in better performance of the system. Note: a previous attempt to enable stats on system.jobs starting in 23.1.0 was unsuccessful, but we have now fixed the oversight.

@rytaft rytaft requested review from mgartner and a team August 3, 2023 19:26
@rytaft rytaft requested a review from a team as a code owner August 3, 2023 19:26
@blathers-crl
Copy link

blathers-crl bot commented Aug 3, 2023

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

@mgartner
Copy link
Collaborator

mgartner commented Aug 4, 2023

Note that the internal executor does not currently use histograms due to #104443 which we can't revert until we get to the bottom of #102954.

This commit fixes an oversight in cockroachdb#102637 which intended to enable
stats collection on the jobs table, but was not successful.

I've manually confirmed that stats are now collected on the jobs table
in a local cluster:
```
  888074673664065537 | AUTO CREATE STATS               | Table statistics refresh for system.public.jobs                         | CREATE STATISTICS __auto__ FROM [15] WITH OPTIONS THROTTLING 0.9 AS OF SYSTEM TIME '-30s'  | root      | succeeded | NULL           | 2023-08-03 19:01:22.343
```

Informs cockroachdb#107405

Release note (performance improvement): We now automatically collect
table statistics on the system.jobs table, which will enable the optimizer
to produce better query plans for internal queries that access the
system.jobs table. This may result in better performance of the system.
Note: a previous attempt to enable stats on system.jobs starting in 23.1.0
was unsuccessful, but we have now fixed the oversight.
Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Thanks. That shouldn't affect the query in #107405 though, right?

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

Copy link
Collaborator

@DrewKimball DrewKimball 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 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

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:

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

Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

TFTRs!

bors r+

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

@craig
Copy link
Contributor

craig bot commented Aug 5, 2023

Build failed:

Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Known flake: #108221

bors r+

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

@craig
Copy link
Contributor

craig bot commented Aug 5, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 5, 2023

Build failed:

Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Flake: #108247

bors r+

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

@craig
Copy link
Contributor

craig bot commented Aug 5, 2023

Build failed:

@rytaft
Copy link
Collaborator Author

rytaft commented Aug 5, 2023

#108221

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 5, 2023

Build succeeded:

@craig craig bot merged commit f1623e0 into cockroachdb:master Aug 5, 2023
2 checks passed
@mgartner
Copy link
Collaborator

mgartner commented Aug 7, 2023

Thanks. That shouldn't affect the query in #107405 though, right?

I think it does. That query uses the internal executor AFAIK.

@rytaft
Copy link
Collaborator Author

rytaft commented Aug 7, 2023

Thanks. That shouldn't affect the query in #107405 though, right?

I think it does. That query uses the internal executor AFAIK.

I see -- looks like you're right. In that case, probably not worth backporting this change.

@mgartner
Copy link
Collaborator

mgartner commented Aug 7, 2023

I think once we have a root cause for #102954 we can re-evaluate backporting.

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.

5 participants