Skip to content

Commit

Permalink
sql: fix usages of ActiveVersionOrEmpty in {create, alter}_table.go
Browse files Browse the repository at this point in the history
Fixes cockroachdb#45519.

This PR removes an incorrect usage of ActiveVersionOrEmpty in
alter_table.go, and updates a comment in create_table.go detailing its
usage.

Release note: None
  • Loading branch information
rohany committed Mar 3, 2020
1 parent 3e56193 commit 66d502f
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 12 deletions.
5 changes: 2 additions & 3 deletions pkg/sql/alter_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,7 @@ func (n *alterTableNode) startExec(params runParams) error {

case *tree.AlterTableAlterPrimaryKey:
// Make sure that all nodes in the cluster are able to perform primary key changes before proceeding.
version := params.p.ExecCfg().Settings.Version.ActiveVersionOrEmpty(params.ctx)
if !version.IsActive(clusterversion.VersionPrimaryKeyChanges) {
if !params.p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.VersionPrimaryKeyChanges) {
return pgerror.Newf(pgcode.FeatureNotSupported,
"all nodes are not the correct version for primary key changes")
}
Expand All @@ -358,7 +357,7 @@ func (n *alterTableNode) startExec(params runParams) error {
}

if t.Sharded != nil {
if !version.IsActive(clusterversion.VersionHashShardedIndexes) {
if !params.p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.VersionHashShardedIndexes) {
return invalidClusterForShardedIndexError
}
if !params.p.EvalContext().SessionData.HashShardedIndexesEnabled {
Expand Down
16 changes: 7 additions & 9 deletions pkg/sql/create_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -1181,14 +1181,12 @@ func MakeTableDesc(
// If all nodes in the cluster know how to handle secondary indexes with column families,
// write the new version into new index descriptors.
indexEncodingVersion := sqlbase.BaseIndexFormatVersion
// We can't use st.Version.IsActive because this method is sometimes called
// before the version has been initialized, leading to a panic. There are also
// cases where this function is called in tests where st is nil.
if st != nil {
if version := st.Version.ActiveVersionOrEmpty(ctx); version != (clusterversion.ClusterVersion{}) &&
version.IsActive(clusterversion.VersionSecondaryIndexColumnFamilies) {
indexEncodingVersion = sqlbase.SecondaryIndexFamilyFormatVersion
}
// We can't use st.Version.IsActive because this method is used during
// server setup before the cluster version has been initialized.
version := st.Version.ActiveVersionOrEmpty(ctx)
if version != (clusterversion.ClusterVersion{}) &&
version.IsActive(clusterversion.VersionSecondaryIndexColumnFamilies) {
indexEncodingVersion = sqlbase.SecondaryIndexFamilyFormatVersion
}

for i, def := range n.Defs {
Expand All @@ -1210,7 +1208,7 @@ func MakeTableDesc(
if st == nil {
return desc, invalidClusterForShardedIndexError
}
if version := st.Version.ActiveVersionOrEmpty(ctx); version == (clusterversion.ClusterVersion{}) ||
if version == (clusterversion.ClusterVersion{}) ||
!version.IsActive(clusterversion.VersionHashShardedIndexes) {
return desc, invalidClusterForShardedIndexError
}
Expand Down

0 comments on commit 66d502f

Please sign in to comment.