Skip to content

Commit

Permalink
sql: fix migration with new system.table_statistics column
Browse files Browse the repository at this point in the history
Before this change, the new `system.table_statistics` column `avgSize`
introduced 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: cockroachdb#77979

Release note (sql): Fixes a migration discrepancy from 22.1.10 to
22.1.12 where the column ordering differed between the system schema and
the table after migration. Additionally, it ensures that during
migration the column added as part of the migration is added to an
existing column family.
  • Loading branch information
rharding6373 committed Mar 23, 2022
1 parent e02793d commit 71154e4
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 7 deletions.
3 changes: 2 additions & 1 deletion pkg/migration/migrations/alter_table_statistics_avg_size.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import (

const addAvgSizeCol = `
ALTER TABLE system.table_statistics
ADD COLUMN IF NOT EXISTS "avgSize" INT8 NOT NULL DEFAULT (INT8 '0')
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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},
}
)

Expand Down
1 change: 1 addition & 0 deletions pkg/migration/migrations/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
var (
HasColumn = hasColumn
HasIndex = hasIndex
HasColumnFamily = hasColumnFamily
CreateSystemTable = createSystemTable
)

Expand Down
44 changes: 44 additions & 0 deletions pkg/migration/migrations/schema_changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
12 changes: 6 additions & 6 deletions pkg/sql/catalog/systemschema/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand All @@ -1327,8 +1327,8 @@ var (
"rowCount",
"distinctCount",
"nullCount",
"avgSize",
"histogram",
"avgSize",
},
ColumnIDs: []descpb.ColumnID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
},
Expand Down

0 comments on commit 71154e4

Please sign in to comment.