Skip to content

stats: use table descriptors instead of IDs#68997

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
postamar:add-descs-to-stats-methods
Aug 17, 2021
Merged

stats: use table descriptors instead of IDs#68997
craig[bot] merged 1 commit intocockroachdb:masterfrom
postamar:add-descs-to-stats-methods

Conversation

@postamar
Copy link
Copy Markdown

@postamar postamar commented Aug 16, 2021

Previously, the sql stats package inferred various properties about
a table from its ID, like if it is a system or a virtual table.
However the table descriptor is usually readily or easily available,
providing much richer information about a table than its ID.
In particular, this allows us to stop collecting stats for views.

Release note (sql change): table statistics are no longer collected
for views.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@postamar postamar force-pushed the add-descs-to-stats-methods branch 3 times, most recently from f38b84e to 731389b Compare August 16, 2021 19:44
@postamar postamar force-pushed the add-descs-to-stats-methods branch from 731389b to d8847c7 Compare August 16, 2021 23:09
@postamar postamar marked this pull request as ready for review August 17, 2021 00:30
@postamar postamar requested review from a team August 17, 2021 00:30
@postamar postamar requested a review from a team as a code owner August 17, 2021 00:30
@postamar postamar requested review from shermanCRL and removed request for a team August 17, 2021 00:30
@shermanCRL shermanCRL requested review from a team and removed request for shermanCRL August 17, 2021 00:54
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

LGTM

@dt dt removed the request for review from a team August 17, 2021 01:19
Copy link
Copy Markdown
Collaborator

@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.

:lgtm:

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


pkg/sql/stats/automatic_stats.go, line 380 at r1 (raw file):

func (r *Refresher) NotifyMutation(table catalog.TableDescriptor, rowsAffected int) {
	if !AutomaticStatisticsClusterMode.Get(&r.st.SV) || !hasStatistics(table) {
		// Automatic stats are disabled.

nit: tweak comment to include your change


pkg/sql/stats/stats_cache.go, line 239 at r1 (raw file):

		return false
	}
	if table.IsView() {

nit: you're already checking table.IsView() in the block above

@postamar postamar force-pushed the add-descs-to-stats-methods branch from d8847c7 to 7213dba Compare August 17, 2021 09:44
@postamar
Copy link
Copy Markdown
Author

postamar commented Aug 17, 2021

Thanks for the reviews. I've applied the requested changes.

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 17, 2021

Build failed:

Previously, the sql stats package inferred various properties about
a table from its ID, like if it is a system or a virtual table.
However the table descriptor is usually readily or easily available,
providing much richer information about a table than its ID.
In particular, this allows us to stop collecting stats for views.

Release note (sql change): table statistics are no longer collected
for views.
@postamar postamar force-pushed the add-descs-to-stats-methods branch from 7213dba to 7273596 Compare August 17, 2021 12:33
@postamar
Copy link
Copy Markdown
Author

bors r+

Sorry for the noise, I'd checked in a stupid typo.

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 17, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 17, 2021

Build succeeded:

@craig craig bot merged commit b69e790 into cockroachdb:master Aug 17, 2021
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.

4 participants