sql: fix migration with new system.table_statistics column #78302
Conversation
|
This change is |
rharding6373
force-pushed
the
table_stats_schema
branch
from
71154e4 to
e925a85
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @postamar, @rytaft, and @yuzefovich)
-- commits, line 18 at r2:
Question for @postamar: Do I need need to do any special migration for any version that may have already migrated to internal version 21.2.12 or later? I don't think so, because 22.1 hasn't been released and this change will need to be backported, but I want to double check...
pkg/sql/catalog/systemschema/system.go, line 1314 at r2 (raw file):
{Name: "distinctCount", ID: 7, Type: types.Int}, {Name: "nullCount", ID: 8, Type: types.Int}, {Name: "histogram", ID: 9, Type: types.Bytes, Nullable: true},
I did check that all calls to this table use column names explicitly and do not rely on the ordering (with the exception of backup_test.go, which has a SELECT * but doesn't use the values so it's ok), so changing the order shouldn't require any other code changes.
|
What do you mean by |
There was a problem hiding this comment.
LGTM, but I'll defer to @cockroachdb/sql-schema for approval.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:
-- commits, line 17 at r2:
No need for release note since we haven't had a beta of 22.1.0 yet.
Previously, rharding6373 (Rachael Harding) wrote…
Question for @postamar: Do I need need to do any special migration for any version that may have already migrated to internal version 21.2.12 or later? I don't think so, because 22.1 hasn't been released and this change will need to be backported, but I want to double check...
Since we haven't had a beta yet, it's ok to not do anything - we don't provide any guarantee that a cluster running an alpha version can be upgraded.
pkg/migration/migrations/alter_table_statistics_avg_size.go, line 24 at r2 (raw file):
const addAvgSizeCol = ` ALTER TABLE system.table_statistics
nit: dangling spaces.
|
RE:
Sorry, this was a typo. The internal versions are 21.2.10 and 21.2.12. Yahor answered my question, which was about whether there is any additional care that needs to be taken when updating the migration between those versions. |
rharding6373
force-pushed
the
table_stats_schema
branch
from
e925a85 to
79fb4fe
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
No need for release note since we haven't had a beta of 22.1.0 yet.
Done.
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Since we haven't had a beta yet, it's ok to not do anything - we don't provide any guarantee that a cluster running an alpha version can be upgraded.
Thanks!
Before this change, the new `system.table_statistics` column `avgSize` introduced in version 21.2.12 was appended to the end of the table during migration, but the system schema had the new column in a different order. The column was also not added to the existing column family containing all table columns during migration. This change fixes both the system schema and the migration commands so that the column ordering is the same and the new column is added to the existing column family. Unfortunately, this means that the existing column family name is unable to be updated to include the column. Fixes: cockroachdb#77979 Release justification: Fixes a schema migration bug in the table_statistics table. Release note: None
rharding6373
force-pushed
the
table_stats_schema
branch
from
79fb4fe to
40ffa99
Compare
|
TFTRs! bors r+ |
|
bors r+ |
|
Build succeeded: |
Previously, the migration code for system table `statement_diagnostics_requests` results in discrepancies when compared to the table definition after bootstrapping. The discrepanies are summarized in cockroachdb#77980. This PR fixes it to ensure this table has exactly the same definition after cluster upgrading as that from bootstrapping. Release note (bug fix): Ensure the migration logic for `system.statement_diagnostics_requests` results in a table definition that is identical to that from bootstrapping. Fixes: cockroachdb#77980 Sister Issue: cockroachdb#78302
78585: kv: ignore unevaluated Get requests after optimistic evaluation r=nvanbenschoten a=nvanbenschoten Related to but distinct from #78584. This commit adds logic to avoid checking for optimistic evaluation conflicts for Get requests which were not evaluated due to a batch-level byte or key limit. This parallels existing logic that does the same for Scan and ReverseScan requests. This was likely missed in the past because we only recently began using Get requests in more places. The effect of this change is that limited batches of point reads will conflict less with writes, reducing read/write contention. Batches of this form could be issued by a statement that looks like: `SELECT * FROM t WHERE id IN (1, 3, 5) LIMIT 2`. 78618: ccl/sqlproxyccl: add connection tracker component r=JeffSwenson a=jaylim-crl #### ccl/sqlproxyccl: add connection tracker component This commit adds a connection tracker component to the balancer, and this will be used to track all instances of forwarders for each tenant. The forwarders can then be used for connection migration. Release note: None #### ccl/sqlproxyccl: make forwarder implement balancer.ConnectionHandle This commit makes the forwarder struct implement the balancer.ConnectionHandle interface. This then allows the forwarder to be stored within the connection tracker component. Release note: None #### ccl/sqlproxyccl: register forwarders with the connection tracker This commit registers all forwarders with the connection tracker. At the same time, we remove the afterForward testing hook since that is no longer needed. Release note: None 78633: roachtest: exit 11 after cluster provisioning failures r=srosenberg a=tbg `roachtest` exits nonzero if it was unable to run any tests due to cloud flakiness (resource exhaustion etc). Previously the "nonzero" was 1 which is the catchall exit code. Now it uses 11 which can now signal this specific failure mode, and could be special cased in our CI scripts if we so desire in the future. Release note: None 78679: migrations: Fix migration discrepancy on a system table. r=Xiang-Gu a=Xiang-Gu Previously, the migration code for system table `statement_diagnostics_requests` results in discrepancies when compared to the table definition after bootstrapping. The discrepancies are summarized in #77980. This PR fixes it to ensure this table has exactly the same definition after cluster upgrading as that from bootstrapping. Release note: None Fixes: #77980 Sister Issue: #78302 Related PR that discovers this discrepancy: #77970 Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Jay <[email protected]> Co-authored-by: Tobias Grieger <[email protected]> Co-authored-by: Xiang Gu <[email protected]>
Before this change, the new
system.table_statisticscolumnavgSizeintroduced in version 22.1.12 was appended to the end of the table
during migration, but the system schema had the new column in a
different order. The column was also not added to the existing column
family containing all table columns during migration.
This change fixes both the system schema and the migration commands so
that the column ordering is the same and the new column is added to the
existing column family. Unfortunately, this means that the existing
column family name is unable to be updated to include the column.
Fixes: #77979
Release justification: Fixes a schema migration bug in the
table_statistics table.
Release note: None