From 40ffa9935456a2f5cddffe694af4a25d158748c6 Mon Sep 17 00:00:00 2001 From: rharding6373 Date: Tue, 22 Mar 2022 19:08:51 -0700 Subject: [PATCH] sql: fix migration with new system.table_statistics column 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: #77979 Release justification: Fixes a schema migration bug in the table_statistics table. Release note: None --- .../alter_table_statistics_avg_size.go | 3 +- .../alter_table_statistics_avg_size_test.go | 2 + pkg/migration/migrations/helpers_test.go | 1 + pkg/migration/migrations/schema_changes.go | 44 +++++++++++++++++++ pkg/sql/catalog/systemschema/system.go | 12 ++--- .../testdata/logic_test/information_schema | 8 ++-- 6 files changed, 59 insertions(+), 11 deletions(-) diff --git a/pkg/migration/migrations/alter_table_statistics_avg_size.go b/pkg/migration/migrations/alter_table_statistics_avg_size.go index e0cf87d2e6d9..3e6cda4b40d7 100644 --- a/pkg/migration/migrations/alter_table_statistics_avg_size.go +++ b/pkg/migration/migrations/alter_table_statistics_avg_size.go @@ -21,8 +21,9 @@ import ( ) const addAvgSizeCol = ` -ALTER TABLE system.table_statistics +ALTER TABLE system.table_statistics ADD COLUMN IF NOT EXISTS "avgSize" INT8 NOT NULL DEFAULT (INT8 '0') +FAMILY "fam_0_tableID_statisticID_name_columnIDs_createdAt_rowCount_distinctCount_nullCount_histogram" ` func alterSystemTableStatisticsAddAvgSize( diff --git a/pkg/migration/migrations/alter_table_statistics_avg_size_test.go b/pkg/migration/migrations/alter_table_statistics_avg_size_test.go index 5f7aa888a3af..d43e1e9efe30 100644 --- a/pkg/migration/migrations/alter_table_statistics_avg_size_test.go +++ b/pkg/migration/migrations/alter_table_statistics_avg_size_test.go @@ -58,6 +58,8 @@ func TestAlterSystemTableStatisticsTable(t *testing.T) { var ( validationSchemas = []migrations.Schema{ {Name: "avgSize", ValidationFn: migrations.HasColumn}, + {Name: "fam_0_tableID_statisticID_name_columnIDs_createdAt_rowCount_distinctCount_nullCount_histogram", + ValidationFn: migrations.HasColumnFamily}, } ) diff --git a/pkg/migration/migrations/helpers_test.go b/pkg/migration/migrations/helpers_test.go index b9ab2ec19897..aca53e92c2e7 100644 --- a/pkg/migration/migrations/helpers_test.go +++ b/pkg/migration/migrations/helpers_test.go @@ -31,6 +31,7 @@ import ( var ( HasColumn = hasColumn HasIndex = hasIndex + HasColumnFamily = hasColumnFamily CreateSystemTable = createSystemTable ) diff --git a/pkg/migration/migrations/schema_changes.go b/pkg/migration/migrations/schema_changes.go index cd8448ca96d8..37ac41c74866 100644 --- a/pkg/migration/migrations/schema_changes.go +++ b/pkg/migration/migrations/schema_changes.go @@ -248,3 +248,47 @@ func hasIndex(storedTable, expectedTable catalog.TableDescriptor, indexName stri } return true, nil } + +// hasColumnFamily returns true if storedTable already has the given column +// family, comparing with expectedTable. storedTable descriptor must be read +// from system storage as compared to reading from the systemschema package. On +// the contrary, expectedTable must be accessed directly from systemschema +// package. This function returns an error if the column doesn't exist in the +// expectedTable descriptor. +func hasColumnFamily( + storedTable, expectedTable catalog.TableDescriptor, colFamily string, +) (bool, error) { + var storedFamily, expectedFamily *descpb.ColumnFamilyDescriptor + for _, fam := range storedTable.GetFamilies() { + if fam.Name == colFamily { + storedFamily = &fam + break + } + } + if storedFamily == nil { + return false, nil + } + + for _, fam := range expectedTable.GetFamilies() { + if fam.Name == colFamily { + expectedFamily = &fam + break + } + } + if expectedFamily == nil { + return false, errors.Errorf("column family %s does not exist", colFamily) + } + + // Check that columns match. + storedFamilyCols := storedFamily.ColumnNames + expectedFamilyCols := expectedFamily.ColumnNames + if len(storedFamilyCols) != len(expectedFamilyCols) { + return false, nil + } + for i, storedCol := range storedFamilyCols { + if storedCol != expectedFamilyCols[i] { + return false, nil + } + } + return true, nil +} diff --git a/pkg/sql/catalog/systemschema/system.go b/pkg/sql/catalog/systemschema/system.go index c2a51fabfd73..389b3848d56c 100644 --- a/pkg/sql/catalog/systemschema/system.go +++ b/pkg/sql/catalog/systemschema/system.go @@ -251,10 +251,10 @@ CREATE TABLE system.table_statistics ( "rowCount" INT8 NOT NULL, "distinctCount" INT8 NOT NULL, "nullCount" INT8 NOT NULL, - "avgSize" INT8 NOT NULL DEFAULT 0, histogram BYTES, + "avgSize" INT8 NOT NULL DEFAULT 0, CONSTRAINT "primary" PRIMARY KEY ("tableID", "statisticID"), - FAMILY ("tableID", "statisticID", name, "columnIDs", "createdAt", "rowCount", "distinctCount", "nullCount", "avgSize", histogram) + FAMILY "fam_0_tableID_statisticID_name_columnIDs_createdAt_rowCount_distinctCount_nullCount_histogram" ("tableID", "statisticID", name, "columnIDs", "createdAt", "rowCount", "distinctCount", "nullCount", histogram, "avgSize") );` // locations are used to map a locality specified by a node to geographic @@ -1311,12 +1311,12 @@ var ( {Name: "rowCount", ID: 6, Type: types.Int}, {Name: "distinctCount", ID: 7, Type: types.Int}, {Name: "nullCount", ID: 8, Type: types.Int}, - {Name: "avgSize", ID: 9, Type: types.Int, DefaultExpr: &zeroIntString}, - {Name: "histogram", ID: 10, Type: types.Bytes, Nullable: true}, + {Name: "histogram", ID: 9, Type: types.Bytes, Nullable: true}, + {Name: "avgSize", ID: 10, Type: types.Int, DefaultExpr: &zeroIntString}, }, []descpb.ColumnFamilyDescriptor{ { - Name: "fam_0_tableID_statisticID_name_columnIDs_createdAt_rowCount_distinctCount_nullCount_avgSize_histogram", + Name: "fam_0_tableID_statisticID_name_columnIDs_createdAt_rowCount_distinctCount_nullCount_histogram", ID: 0, ColumnNames: []string{ "tableID", @@ -1327,8 +1327,8 @@ var ( "rowCount", "distinctCount", "nullCount", - "avgSize", "histogram", + "avgSize", }, ColumnIDs: []descpb.ColumnID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}, }, diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 123b21b8f90e..4e5b81d6f4e2 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -1601,6 +1601,7 @@ system public 630200280_42_8_not_null system public 630200280_42_9_not_null system public statement_statistics CHECK NO NO system public check_crdb_internal_aggregated_ts_app_name_fingerprint_id_node_id_plan_hash_transaction_fingerprint_id_shard_8 system public statement_statistics CHECK NO NO system public primary system public statement_statistics PRIMARY KEY NO NO +system public 630200280_20_10_not_null system public table_statistics CHECK NO NO system public 630200280_20_1_not_null system public table_statistics CHECK NO NO system public 630200280_20_2_not_null system public table_statistics CHECK NO NO system public 630200280_20_4_not_null system public table_statistics CHECK NO NO @@ -1608,7 +1609,6 @@ system public 630200280_20_5_not_null system public 630200280_20_6_not_null system public table_statistics CHECK NO NO system public 630200280_20_7_not_null system public table_statistics CHECK NO NO system public 630200280_20_8_not_null system public table_statistics CHECK NO NO -system public 630200280_20_9_not_null system public table_statistics CHECK NO NO system public primary system public table_statistics PRIMARY KEY NO NO system public 630200280_50_1_not_null system public tenant_settings CHECK NO NO system public 630200280_50_2_not_null system public tenant_settings CHECK NO NO @@ -1693,6 +1693,7 @@ system public 630200280_19_3_not_null system public 630200280_19_4_not_null createdAt IS NOT NULL system public 630200280_19_5_not_null expiresAt IS NOT NULL system public 630200280_19_7_not_null lastUsedAt IS NOT NULL +system public 630200280_20_10_not_null avgSize IS NOT NULL system public 630200280_20_1_not_null tableID IS NOT NULL system public 630200280_20_2_not_null statisticID IS NOT NULL system public 630200280_20_4_not_null columnIDs IS NOT NULL @@ -1700,7 +1701,6 @@ system public 630200280_20_5_not_null system public 630200280_20_6_not_null rowCount IS NOT NULL system public 630200280_20_7_not_null distinctCount IS NOT NULL system public 630200280_20_8_not_null nullCount IS NOT NULL -system public 630200280_20_9_not_null avgSize IS NOT NULL system public 630200280_21_1_not_null localityKey IS NOT NULL system public 630200280_21_2_not_null localityValue IS NOT NULL system public 630200280_21_3_not_null latitude IS NOT NULL @@ -2150,11 +2150,11 @@ system public statement_statistics plan system public statement_statistics plan_hash 4 system public statement_statistics statistics 9 system public statement_statistics transaction_fingerprint_id 3 -system public table_statistics avgSize 9 +system public table_statistics avgSize 10 system public table_statistics columnIDs 4 system public table_statistics createdAt 5 system public table_statistics distinctCount 7 -system public table_statistics histogram 10 +system public table_statistics histogram 9 system public table_statistics name 3 system public table_statistics nullCount 8 system public table_statistics rowCount 6