From b2e4fb5666fd3845098c8b94fca3f424626d8b37 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 17 Jan 2022 18:11:25 -0800 Subject: [PATCH] sql: clean up mutable not-null columns hack Mutation columns in some cases need to be scanned even if they haven't been backfilled yet, which means that we may retrieve NULL values even if they are market as not-nullable. We currently have a hack in the table descriptor which changes the nullable flags in the column descriptors when `ReadableColumns()` is used. It is very surprising that we can get different descriptors for a given ColumnID depending if we look for it in `ReadableColumns()` or in `AllColumns()` (e.g. via FindColumnWithID. This commit cleans this up, changing the scanning code to check for `Public()` instead. Release note: None --- pkg/sql/catalog/descriptor.go | 5 ++--- pkg/sql/catalog/tabledesc/column.go | 12 ++---------- pkg/sql/colfetcher/cfetcher.go | 2 +- pkg/sql/row/fetcher.go | 11 ++++++----- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 2473b192e7fc..2f7943667a47 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -447,9 +447,8 @@ type TableDescriptor interface { // details. AccessibleColumns() []Column // ReadableColumns is a list of columns (including those undergoing a schema - // change) which can be scanned. Columns in the process of a schema change - // are all set to nullable while column backfilling is still in - // progress, as mutation columns may have NULL values. + // change) which can be scanned. Note that mutation columns may produce NULL + // values when scanned, even if they are marked as not nullable. ReadableColumns() []Column // UserDefinedTypeColumns returns a slice of Column interfaces // containing the table's columns with user defined types, in the diff --git a/pkg/sql/catalog/tabledesc/column.go b/pkg/sql/catalog/tabledesc/column.go index 18020b0ff03e..f51d1ccec646 100644 --- a/pkg/sql/catalog/tabledesc/column.go +++ b/pkg/sql/catalog/tabledesc/column.go @@ -315,8 +315,8 @@ func newColumnCache(desc *descpb.TableDescriptor, mutations *mutationCache) *col c.writable = c.public c.nonDrop = c.public } else { - readableDescs := make([]descpb.ColumnDescriptor, 0, numMutations) - readableBackingStructs := make([]column, 0, numMutations) + //readableDescs := make([]descpb.ColumnDescriptor, 0, numMutations) + //readableBackingStructs := make([]column, 0, numMutations) for _, col := range c.deletable { if !col.DeleteOnly() { lazyAllocAppendColumn(&c.writable, col, numDeletable) @@ -324,14 +324,6 @@ func newColumnCache(desc *descpb.TableDescriptor, mutations *mutationCache) *col if !col.Dropped() { lazyAllocAppendColumn(&c.nonDrop, col, numDeletable) } - if !col.Public() && !col.IsNullable() { - j := len(readableDescs) - readableDescs = append(readableDescs, *col.ColumnDesc()) - readableDescs[j].Nullable = true - readableBackingStructs = append(readableBackingStructs, *col.(*column)) - readableBackingStructs[j].desc = &readableDescs[j] - col = &readableBackingStructs[j] - } lazyAllocAppendColumn(&c.readable, col, numDeletable) } } diff --git a/pkg/sql/colfetcher/cfetcher.go b/pkg/sql/colfetcher/cfetcher.go index aa7956c4685e..e97b28ae03e9 100644 --- a/pkg/sql/colfetcher/cfetcher.go +++ b/pkg/sql/colfetcher/cfetcher.go @@ -1379,7 +1379,7 @@ func (rf *cFetcher) fillNulls() error { if table.compositeIndexColOrdinals.Contains(i) { continue } - if !table.cols[i].IsNullable() { + if !table.cols[i].IsNullable() && table.cols[i].Public() { var indexColValues strings.Builder rf.writeDecodedCols(&indexColValues, table.indexColOrdinals, ',') return scrub.WrapError(scrub.UnexpectedNullValueError, errors.Errorf( diff --git a/pkg/sql/row/fetcher.go b/pkg/sql/row/fetcher.go index b615c55d2f87..841d7f0278fd 100644 --- a/pkg/sql/row/fetcher.go +++ b/pkg/sql/row/fetcher.go @@ -1215,15 +1215,16 @@ func (rf *Fetcher) finalizeRow() error { } // Fill in any missing values with NULLs - for i := range table.cols { + for i, col := range table.cols { if rf.valueColsFound == table.neededValueCols { // Found all cols - done! return nil } - if table.neededCols.Contains(int(table.cols[i].GetID())) && table.row[i].IsUnset() { + if table.neededCols.Contains(int(col.GetID())) && table.row[i].IsUnset() { // If the row was deleted, we'll be missing any non-primary key - // columns, including nullable ones, but this is expected. - if !table.cols[i].IsNullable() && !table.rowIsDeleted && !rf.IgnoreUnexpectedNulls { + // columns, including nullable ones, but this is expected. If the column + // is not yet active, we can also expect NULLs. + if !col.IsNullable() && !table.rowIsDeleted && !rf.IgnoreUnexpectedNulls && col.Public() { var indexColValues []string for _, idx := range table.indexColIdx { if idx != -1 { @@ -1234,7 +1235,7 @@ func (rf *Fetcher) finalizeRow() error { } return errors.AssertionFailedf( "Non-nullable column \"%s:%s\" with no value! Index scanned was %q with the index key columns (%s) and the values (%s)", - table.desc.GetName(), table.cols[i].GetName(), table.index.GetName(), + table.desc.GetName(), col.GetName(), table.index.GetName(), strings.Join(table.index.IndexDesc().KeyColumnNames, ","), strings.Join(indexColValues, ",")) } table.row[i] = rowenc.EncDatum{