sql/backfill: fix bug adding new columns to new index with volatile default #81486

ajwerner · 2022-05-18T22:27:20Z

In #58990 we added support for the index backfiller to evaluate expressions. This unblocked the usage of virtual computed columns in secondary expressions, so wasn't a totally bogus change, but probably was trying to do too much without understanding all of the implications. The original motivation for that change was to unblock the then nascent declarative schema changer prototype which wanted to only implement #47989 as a column change protocol. In order to do that, it needed to evaluate default expressions when adding new columns to a new primary index. For whatever reason, probably lack of understanding, I made it such that all default expressions for columns which were in the adding state were evaluated. This is wrong in cases where the column has already been backfilled into the current primary index; it's wrong because volatile expressions will evaluate to different values causing the newly backfilled secondary index to be bogus and corrupt.

This change addresses the failure of the above change by being more thoughtful about whether it's sane to evaluate a default expression when backfilling into an index. Note that it's still insane to backfill a volatile expression for a new column into the key of a new primary index. As of writing, there is no way to do this.

Backports will address #81448.

Release note (bug fix): In 21.1 a bug was introduced whereby default values
were recomputed when populating data in new secondary indexes for columns
which were added in the same transaction as the index. This arises, for example
in cases like ALTER TABLE t ADD COLUMN f FLOAT8 UNIQUE DEFAULT (random()).
If the default expression is not volatile, then the recomputation is harmless.
If, however, the default expression is volatile, the data in the secondary
index will not match the data in the primary index: a corrupt index will have
been created. This bug has now been fixed.

cockroach-teamcity · 2022-05-18T22:27:26Z

This change is

stevendanna

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @stevendanna)

a discussion (no related file):
Overall I like that you moved some of this logic out so it can be unit tested, and the additional comments also help.

I mostly just left questions I had while reading this.

pkg/sql/backfill/index_backfiller_cols.go line 21 at r1 (raw file):

// indexBackfillerCols holds the interned set of columns used by the
// indexBackfiller, and the information about which columns will need to

[nit] s/, and/ and/ ?

pkg/sql/backfill/index_backfiller_cols.go line 35 at r2 (raw file):

	// the index being backfilled is a new primary index and the columns do not
	// exist in the currently public primary index.
	addedCols []catalog.Column

Is this case possible in the non-declarative schema changer? I believe we block primary index swaps with any other concurrent schema changes, but am I missing another way to get this case in the non-declarative schema changer?

pkg/sql/logictest/testdata/logic_test/alter_table line 2500 at r1 (raw file):


statement ok
create table t81448_b as select b from t81448

Out of curiosity, is this guaranteed to always scan the primary index rather than the secondary index? Looking at the current implementation I believe it will, but is there any reasons we couldn't in theory scan the secondary index on b.

pkg/sql/backfill/index_backfiller_cols_test.go line 191 at r1 (raw file):

			// This is the case where we're building a new primary index as part
			// of an add column.
			name: "one virtual, one new mutation column in source used in new primary",

Looks like this case might be duplicated from above.

ajwerner · 2022-05-19T13:22:36Z

Note that it's still insane to backfill a volatile expression for a new column into the key of a new primary index. As of writing, there is no way to do this.

I filed this as follow-up #81521

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

a discussion (no related file):