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: add explicit column family assignments to persisted stats upgrades #99674

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Mar 27, 2023

Fixes #99270.
Part of #99260.

This commit adds explicit column family assignments to the computed column ADD COLUMN statements for
system.statement_statistics and system.transaction_statistics version upgrades.

Epic: none

Release note: None

@ericharmeling ericharmeling requested review from j82w and a team March 27, 2023 15:51
@blathers-crl
Copy link

blathers-crl bot commented Mar 27, 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.

@ericharmeling ericharmeling added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 27, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling
Copy link
Contributor Author

ericharmeling commented Mar 27, 2023

With #99587, the backport of this PR should fix #99260.

@ericharmeling ericharmeling requested a review from a team March 27, 2023 18:06
Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)


pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 155 at r1 (raw file):

`

const addP99LatencyComputedColTxnStats = `

Can the upgrade test validate the family? Trying to think of some way to test this.

Part of cockroachdb#99260.

This commit adds explicit column family assignments to the
computed column `ADD COLUMN` statements for
system.statement_statistics and system.transaction_statistics
version upgrades.

Epic: none

Release note: None
@ericharmeling ericharmeling force-pushed the system-sql-stats-upgrade-column-family-assignments branch from 14b6b42 to 8c5aee8 Compare March 27, 2023 19:30
Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

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


pkg/upgrade/upgrades/create_computed_indexes_sql_statistics.go line 155 at r1 (raw file):

Previously, j82w (Jake) wrote…

Can the upgrade test validate the family? Trying to think of some way to test this.

I ran the systemschema roach test, which runs in the nightly (how this was caught in the first place). Here's the output (it passes now): systemschema_test_log.txt

Unfortunately, it looks like that's not caught by CI tests, which is how this didn't get caught in the first place.

So I've added another test validation function to TestCreateComputedIndexesOnSystemSQLStatistics that fails on master and succeeds on this branch.

Copy link
Contributor

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

@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 27, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 28, 2023

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Mar 28, 2023

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.


Backport to branch 23.1.x 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-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: systemschema/validate-after-version-upgrade failed
3 participants