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.

Sign up for GitHub

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

Jump to bottom

release-22.2: sql: add crdb_internal.statement_statisticsV22_1 for upgrades #96454

Merged
merged 1 commit into from
Feb 7, 2023
Merged

release-22.2: sql: add crdb_internal.statement_statisticsV22_1 for upgrades #96454

merged 1 commit into from
Feb 7, 2023

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Feb 2, 2023 •
edited
Loading

Previously, the sql activity page would show an error while upgrading
form 22.1 to 22.2 due to the stmt stats endpoing querying from the view
crdb_internal.statement_statistics, due to it reading a system table
column that was newly added in 22.2.

This commit adds the view crdb_internal.statement_statisticsV22_1
which is a copy of crdb_internal.statement_statistics, but without
the newly added index_recommendations column. This view is used to
properly version gate the addition of the index_recommendations column.

Fixes #95587
Epic: none

Release note (bug fix): The sql activity page will no longer crash
(show the error page), when upgrading to 22.2.

Release justification: bug fix

@blathers-crl
Copy link

blathers-crl bot commented Feb 2, 2023 •
edited by xinhaoz
Loading

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • 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.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz changed the title release-22.2: sql: add crdb_internal.statement_statistics_old for mixed versions release-22.2: sql: add crdb_internal.statement_statistics_v22_2 for upgrades Feb 2, 2023
@xinhaoz xinhaoz force-pushed the gate-index-recs branch 4 times, most recently from c6486ab to bd8201d Compare February 6, 2023 18:19
@xinhaoz xinhaoz changed the title release-22.2: sql: add crdb_internal.statement_statistics_v22_2 for upgrades release-22.2: sql: add crdb_internal.statement_statistics_v22_1 for upgrades Feb 6, 2023
@xinhaoz xinhaoz changed the title release-22.2: sql: add crdb_internal.statement_statistics_v22_1 for upgrades release-22.2: sql: add crdb_internal.statement_statisticsV22_1 for upgrades Feb 6, 2023
@xinhaoz xinhaoz marked this pull request as ready for review February 6, 2023 19:02
@xinhaoz xinhaoz requested review from a team February 6, 2023 19:02
@xinhaoz xinhaoz requested a review from a team as a code owner February 6, 2023 19:02
@xinhaoz xinhaoz requested review from a team and removed request for a team February 6, 2023 19:02
maryliag
maryliag approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

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

@xinhaoz
sql: add crdb_internal.statement_statistics_old for mixed versions
c31fdb0 
Previously, the sql activity page would show an error while upgrading
form 22.1 to 22.2 due to the stmt stats endpoing querying from the view
crdb_internal.statement_statistics, due to it reading a system table
column that was newly added in 22.2.

This commit adds the view `crdb_internal.statement_statistics_old`
which is a copy of `crdb_internal.statement_statistics`, but without
the newly added index_recommendations column. This view is used to
properly version gate the addition of the index_recommendations column.

Fixes cockroachdb#95587
Epic: none

Release note (bug fix): The sql activity page will no longer crash
(show the error page), when upgrading to 22.2.
@xinhaoz xinhaoz merged commit a8607fc into cockroachdb:release-22.2 Feb 7, 2023
@xinhaoz xinhaoz deleted the gate-index-recs branch February 7, 2023 18:51
@xinhaoz xinhaoz mentioned this pull request Feb 15, 2023
Merged
xinhaoz added a commit to xinhaoz/cockroach that referenced this pull request Feb 15, 2023
@xinhaoz
release-22.2: logictest: remove added lines in crdb_internal
81d25d8 
cockroachdb#96454 mistakenly added
some extra lines to the crdb_internal logic test causing stress tests
to fail.

Epic: none
Fixes: cockroachdb#96861

Release note: None
@maryliag maryliag mentioned this pull request Apr 4, 2023
Closed
maryliag added a commit to maryliag/cockroach that referenced this pull request Apr 4, 2023
@maryliag
sql: add new persistedV22_2 views
5afb2dc 
Fixes cockroachdb#100501

Adds `{statement|transaction}_statistics_persisted_v22_2`
like they were added on cockroachdb#96454.
Check the version of the cluster before deciding which view
to use.
This is required for mixed version cluster with 22.2 and 23.1 versions.

Release note: None
state between 22.2 and 23.1
@maryliag maryliag mentioned this pull request Apr 5, 2023
Merged
maryliag added a commit to maryliag/cockroach that referenced this pull request Apr 5, 2023
@maryliag
release-23.1: sql: add new persistedV22_2 views
24a2892 
Fixes cockroachdb#100501

Adds `{statement|transaction}_statistics_persisted_v22_2`
like they were added on cockroachdb#96454.
Check the version of the cluster before deciding which view
to use.
This is required for mixed version cluster with 22.2 and 23.1 versions.

Release note: None
maryliag added a commit to maryliag/cockroach that referenced this pull request Apr 5, 2023
@maryliag
release-23.1: sql: add new persistedV22_2 views
ff2ccf2 
Fixes cockroachdb#100501

Adds `{statement|transaction}_statistics_persisted_v22_2`
like they were added on cockroachdb#96454.
Check the version of the cluster before deciding which view
to use.
This is required for mixed version cluster with 22.2 and 23.1 versions.

Release note: None
@maryliag maryliag mentioned this pull request Apr 5, 2023
Merged
maryliag added a commit to maryliag/cockroach that referenced this pull request Apr 5, 2023
@maryliag
sql: add new persistedV22_2 views
f20f31b 
Fixes cockroachdb#100501

Adds {statement|transaction}_statistics_persisted_v22_2 like they were added on cockroachdb#96454.
Check the version of the cluster before deciding which view to use.
This is required for mixed version cluster with 22.2 and 23.1 versions.

Release note: None
maryliag added a commit to maryliag/cockroach that referenced this pull request Apr 5, 2023
@maryliag
sql: add new persistedV22_2 views
3dabaaf 
Fixes cockroachdb#100501

Adds {statement|transaction}_statistics_persisted_v22_2 like they were added on cockroachdb#96454.
Check the version of the cluster before deciding which view to use.
This is required for mixed version cluster with 22.2 and 23.1 versions.

Release note: None
craig bot pushed a commit that referenced this pull request Apr 10, 2023
@adityamaru @maryliag
@msbutler @knz
Merge #100618 #100703 #100734 #100916 #101057
bd2eaeb 
100618: builtins: remove WaitPolicy error from fingerprinting builtin r=dt a=adityamaru

During development the wait policy for ExportRequests sent during fingerprinting was set to error. This meant that if the ExportRequest encountered an intent it would immediately return and error out. This is fine if we added a retry loop similar to how backup processor requeues the request to be sent at a later time by when the intent is resolved, but as is this is incorrect. We change the WaitPolicy to block so that the ExportRequest blocks until the intent is resolved. If in the future we want to make progress while another ExportRequest is stuck resolving intents we can rework this logic to look similar to our backup strategy.

Release note: None
Epic: none

100703: sql: add new persistedV22_2 views r=maryliag a=maryliag

Note to reviewers: this is a forward-porting from #100673

Fixes #100501

Adds {statement|transaction}_statistics_persisted_v22_2 like they were added on #96454. Check the version of the cluster before deciding which view to use. This is required for mixed version cluster with 22.2 and 23.1 versions.

Release note: None

100734: backupccl: gate writing slim manifests in backup via cluster setting r=rhu713 a=rhu713

Feature gate the writing of slim manifests at the end of backup jobs with
the cluster setting `backup.write_metadata_with_external_ssts.enabled`.

Release note: None

100916: roachtest: fix c2c/shutdown/dest bug r=dt,adityamaru a=msbutler

Previously, the c2c/shutdown/dest tests were incorrectly shutting down nodes on the source cluster. This patch fixes this.

Fixes #100907

Release note: None

101057: sqlproxyccl: skip TestDirectoryConnect r=adityamaru a=knz

Informs #76839.

Release note: None
Epic: None

Co-authored-by: adityamaru <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Rui Hu <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryliag maryliag maryliag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xinhaoz @cockroach-teamcity @maryliag