Merge #155778 #155845

155778: sql: add `ALTER TABLE ... DROP STORED` to the declerative schema changer r=shghasemi a=shghasemi

Previously, `ALTER TABLE ... DROP STORED` would run with the legacy schema changer. This change enables the comment be run by the declerative schema changer.

Epic: none

Fixes: #139603

Release note (sql change): `ALTER TABLE ... DROP STORED` will use the declerative schema changer to run.

155845: roachtest: allow local runs of azure backup roachtest r=kev-cao a=msbutler

Previously the azure backup roachtest could only in the nightlies, and not by developers on their machines. This patch removes the clunky credentials auth path the test used to use. Now, the test auths to the bucket via an IAM policy that the developer needs to set up following the internal azure roachprod tutorial.

Epic: none

Release note: none

Co-authored-by: Shadi Ghasemitaheri <[email protected]>
Co-authored-by: Michael Butler <[email protected]>

Author: 3 people

pkg/ccl/schemachangerccl/sctestbackupccl/backup_base_generated_test.go

@@ -354,7 +354,6 @@ go_library(
         "@com_github_prometheus_common//model",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
-        "@in_gopkg_yaml_v3//:yaml_v3",
         "@org_golang_google_protobuf//proto",
         "@org_golang_x_exp//maps",
         "@org_golang_x_oauth2//clientcredentials",

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -65,10 +65,6 @@ const (
 	AssumeRoleGCSCredentials    = "GOOGLE_CREDENTIALS_ASSUME_ROLE"
 	AssumeRoleGCSServiceAccount = "GOOGLE_SERVICE_ACCOUNT"
 
-	AzureClientIDEnvVar     = "AZURE_CLIENT_ID"
-	AzureClientSecretEnvVar = "AZURE_CLIENT_SECRET"
-	AzureTenantIDEnvVar     = "AZURE_TENANT_ID"
-
 	// rows2TiB is the number of rows to import to load 2TB of data (when
 	// replicated).
 	rows2TiB   = 65_104_166

pkg/cmd/roachtest/tests/backup.go

@@ -10,7 +10,6 @@ import (
 	gosql "database/sql"
 	"fmt"
 	"net/url"
-	"os"
 	"path"
 	"time"
 
@@ -34,21 +33,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
-	"gopkg.in/yaml.v3"
 )
 
-// At the moment, Azure VMs do not have managed identities set up yet.
-// Therefore, in order to use implicit authentication, we need to put a
-// credentials file on each VM and point the
-// `COCKROACH_AZURE_APPLICATION_CREDENTIALS_FILE` environment variable at the
-// file.
-// Currently, the only set of credentials that have write access to the storage
-// buckets are the Teamcity credentials, so the Azure fixture roachtests cannot
-// be run locally until those managed identities are set up.
-// TODO (kev-cao): Once managed identities are set up, we can remove this file
-// and rely on the managed identity to authenticate with Azure Blob Storage.
-const azureCredentialsFilePath = "/home/ubuntu/azure-credentials.yaml"
-
 // Maps a fixture database name to the expected number of tables in the
 // database, useful for verifying that the fingerprint of the fixture is as
 // expected.
@@ -197,9 +183,6 @@ func (bd *backupDriver) prepareCluster(ctx context.Context) {
 				"cloudstorage.s3.client_retry_token_bucket.enabled": "false",
 				"cloudstorage.azure.try.timeout":                    "0s",
 			},
-			install.EnvOption{
-				fmt.Sprintf("COCKROACH_AZURE_APPLICATION_CREDENTIALS_FILE=%s", azureCredentialsFilePath),
-			},
 		))
 }
 
@@ -653,7 +636,6 @@ func registerBackupFixtures(r registry.Registry) {
 			Suites:            bf.suites,
 			Skip:              bf.skip,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				require.NoError(t, maybePutAzureCredentialsFile(ctx, c, azureCredentialsFilePath))
 				registry := GetFixtureRegistry(ctx, t, c.Cloud())
 
 				handle, err := registry.Create(ctx, bf.fixture.Name, t.L())
@@ -689,44 +671,6 @@ func registerBackupFixtures(r registry.Registry) {
 	}
 }
 
-func maybePutAzureCredentialsFile(ctx context.Context, c cluster.Cluster, path string) error {
-	if c.Cloud() != spec.Azure {
-		return nil
-	}
-
-	type azureCreds struct {
-		TenantID     string `yaml:"azure_tenant_id"`
-		ClientID     string `yaml:"azure_client_id"`
-		ClientSecret string `yaml:"azure_client_secret"`
-	}
-
-	azureEnvVars := []string{AzureTenantIDEnvVar, AzureClientIDEnvVar, AzureClientSecretEnvVar}
-	azureEnvValues := make(map[string]string)
-	for _, envVar := range azureEnvVars {
-		val := os.Getenv(envVar)
-		if val == "" {
-			return errors.Newf("environment variable %s is not set", envVar)
-		}
-		azureEnvValues[envVar] = val
-	}
-
-	creds := azureCreds{
-		TenantID:     azureEnvValues[AzureTenantIDEnvVar],
-		ClientID:     azureEnvValues[AzureClientIDEnvVar],
-		ClientSecret: azureEnvValues[AzureClientSecretEnvVar],
-	}
-
-	credsYaml, err := yaml.Marshal(creds)
-	if err != nil {
-		return errors.Wrapf(err, "failed to marshal Azure credentials to YAML")
-	}
-
-	return errors.Wrap(
-		c.PutString(ctx, string(credsYaml), path, 0700),
-		"failed to put Azure credentials file in cluster",
-	)
-}
-
 func registerBlobFixtureGC(r registry.Registry) {
 	r.Add(registry.TestSpec{
 		Name:             "blobfixture/gc",

pkg/cmd/roachtest/tests/backup_fixtures.go

@@ -8,6 +8,7 @@ go_library(
         "alter_table_add_column.go",
         "alter_table_add_constraint.go",
         "alter_table_alter_column_drop_not_null.go",
+        "alter_table_alter_column_drop_stored.go",
         "alter_table_alter_column_set_default.go",
         "alter_table_alter_column_set_not_null.go",
         "alter_table_alter_column_set_on_update.go",

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel

@@ -43,6 +43,7 @@ var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{
 	reflect.TypeOf((*tree.AlterTableDropNotNull)(nil)):        {fn: alterTableDropNotNull, on: true, checks: isV253Active},
 	reflect.TypeOf((*tree.AlterTableSetOnUpdate)(nil)):        {fn: alterTableSetOnUpdate, on: true, checks: isV254Active},
 	reflect.TypeOf((*tree.AlterTableRenameColumn)(nil)):       {fn: alterTableRenameColumn, on: true, checks: isV254Active},
+	reflect.TypeOf((*tree.AlterTableDropStored)(nil)):         {fn: alterTableDropStored, on: true, checks: isV261Active},
 }
 
 func init() {

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go

@@ -0,0 +1,69 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package scbuildstmt
+
+import (
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+func alterTableDropStored(
+	b BuildCtx,
+	tn *tree.TableName,
+	tbl *scpb.Table,
+	stmt tree.Statement,
+	t *tree.AlterTableDropStored,
+) {
+	alterColumnPreChecks(b, tn, tbl, t.Column)
+	colElems := b.ResolveColumn(tbl.TableID, t.Column, ResolveParams{
+		RequiredPrivilege: privilege.CREATE,
+	})
+	colElem := colElems.FilterColumn().MustGetOneElement()
+	columnID := colElem.ColumnID
+	// Block alters on system columns.
+	panicIfSystemColumn(colElem, t.Column)
+	// Ensure that the column is stored (not virtual)
+	panicIfVirtualColumn(b, tbl.TableID, columnID, t.Column)
+	// Retrieve the compute expression element
+	exprElem := retrieveColumnComputeExpressionElem(b, tbl.TableID, columnID)
+	// Ensure that the column is computed
+	if exprElem == nil {
+		panic(pgerror.Newf(
+			pgcode.InvalidColumnDefinition,
+			"column %q is not a computed column",
+			tree.ErrString(&t.Column)))
+	}
+	b.Drop(exprElem)
+}
+
+// panicIfVirtualColumn blocks operation if the column is virtual.
+func panicIfVirtualColumn(
+	b BuildCtx, tableID catid.DescID, columnID catid.ColumnID, columnName tree.Name,
+) {
+	colTypeEl := mustRetrieveColumnTypeElem(b, tableID, columnID)
+	if colTypeEl.IsVirtual {
+		panic(pgerror.Newf(
+			pgcode.InvalidColumnDefinition,
+			"column %q is not a stored computed column",
+			tree.ErrString(&columnName)))
+	}
+}
+
+// retrieveColumnComputeExpressionElem returns the compute expression
+// element of the column. Returns nil if no expression exists and for
+// older versions that store the expression as part of the ColumnType
+func retrieveColumnComputeExpressionElem(
+	b BuildCtx, tableID catid.DescID, columnID catid.ColumnID,
+) (expr *scpb.ColumnComputeExpression) {
+	return b.QueryByID(tableID).FilterColumnComputeExpression().Filter(func(
+		_ scpb.Status, _ scpb.TargetStatus, e *scpb.ColumnComputeExpression) bool {
+		return e.ColumnID == columnID
+	}).MustGetZeroOrOneElement()
+}

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_drop_stored.go

@@ -227,3 +227,8 @@ var isV253Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMo
 var isV254Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMode, activeVersion clusterversion.ClusterVersion) bool {
 	return activeVersion.IsActive(clusterversion.V25_4)
 }
+
+var isV261Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMode, activeVersion clusterversion.ClusterVersion) bool {
+	// change the cluster version once V26_1 is created
+	return activeVersion.IsActive(clusterversion.V25_4)
+}

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go

@@ -12,10 +12,6 @@ CREATE TABLE defaultdb.foo (
 );
 ----
 
-unimplemented
-ALTER TABLE defaultdb.foo ALTER COLUMN i DROP STORED
-----
-
 unimplemented
 ALTER TABLE defaultdb.foo RENAME CONSTRAINT foobar TO baz
 ----