From 3da5e7323d4d22677e1ab9d3d527246f41613e09 Mon Sep 17 00:00:00 2001 From: richardjcai Date: Mon, 23 May 2022 19:09:57 -0400 Subject: [PATCH 1/3] roachtest: remove privilege and grant option migration roachtest Was specific to the 21.2 -> 22.2 upgrade, no longer needed on master (22.2) Release note: None --- pkg/cmd/roachtest/tests/BUILD.bazel | 2 - pkg/cmd/roachtest/tests/registry.go | 2 - .../remove_invalid_database_privileges.go | 173 ---------------- .../roachtest/tests/validate_grant_option.go | 184 ------------------ 4 files changed, 361 deletions(-) delete mode 100644 pkg/cmd/roachtest/tests/remove_invalid_database_privileges.go delete mode 100644 pkg/cmd/roachtest/tests/validate_grant_option.go diff --git a/pkg/cmd/roachtest/tests/BUILD.bazel b/pkg/cmd/roachtest/tests/BUILD.bazel index ca1e8e7d30f5..7af0f6f26efc 100644 --- a/pkg/cmd/roachtest/tests/BUILD.bazel +++ b/pkg/cmd/roachtest/tests/BUILD.bazel @@ -97,7 +97,6 @@ go_library( "rapid_restart.go", "rebalance_load.go", "registry.go", - "remove_invalid_database_privileges.go", "replicagc.go", "reset_quorum.go", "restart.go", @@ -138,7 +137,6 @@ go_library( "util_disk_usage.go", "util_if_local.go", "util_load_group.go", - "validate_grant_option.go", "validate_system_schema_after_version_upgrade.go", "version.go", "version_upgrade_public_schema.go", diff --git a/pkg/cmd/roachtest/tests/registry.go b/pkg/cmd/roachtest/tests/registry.go index 788ea63b9ee2..cda41c2a3b7f 100644 --- a/pkg/cmd/roachtest/tests/registry.go +++ b/pkg/cmd/roachtest/tests/registry.go @@ -120,7 +120,6 @@ func RegisterTests(r registry.Registry) { registerKVBench(r) registerTypeORM(r) registerLoadSplits(r) - registerValidateGrantOption(r) registerVersion(r) registerYCSB(r) registerTPCHBench(r) @@ -128,7 +127,6 @@ func RegisterTests(r registry.Registry) { registerMultiTenantUpgrade(r) registerMultiTenantFairness(r) registerVersionUpgradePublicSchema(r) - registerRemoveInvalidDatabasePrivileges(r) registerValidateSystemSchemaAfterVersionUpgrade(r) registerIndexBackfill(r) } diff --git a/pkg/cmd/roachtest/tests/remove_invalid_database_privileges.go b/pkg/cmd/roachtest/tests/remove_invalid_database_privileges.go deleted file mode 100644 index 6b279a7dc1c6..000000000000 --- a/pkg/cmd/roachtest/tests/remove_invalid_database_privileges.go +++ /dev/null @@ -1,173 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package tests - -import ( - "context" - "fmt" - - "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" - "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" - "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" - "github.com/cockroachdb/cockroach/pkg/util/version" - "github.com/stretchr/testify/require" -) - -func registerRemoveInvalidDatabasePrivileges(r registry.Registry) { - r.Add(registry.TestSpec{ - Name: "remove-invalid-database-privileges", - Owner: registry.OwnerSQLExperience, - Cluster: r.MakeClusterSpec(3), - Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { - runRemoveInvalidDatabasePrivileges(ctx, t, c, *t.BuildVersion()) - }, - }) -} - -func runRemoveInvalidDatabasePrivileges( - ctx context.Context, t test.Test, c cluster.Cluster, buildVersion version.Version, -) { - // This is meant to test a migration from 21.2 to 22.1. - if buildVersion.Major() != 22 || buildVersion.Minor() != 1 { - t.L().PrintfCtx(ctx, "skipping test because build version is %s", buildVersion) - return - } - const mainVersion = "" - // We kick off our test on version 21.1.12 since 21.1 was the last version - // we supported granting invalid database privileges. - const version21_1_12 = "21.1.12" - const version21_2_4 = "21.2.4" - u := newVersionUpgradeTest(c, - uploadAndStart(c.All(), version21_1_12), - waitForUpgradeStep(c.All()), - preventAutoUpgradeStep(1), - preventAutoUpgradeStep(2), - preventAutoUpgradeStep(3), - - // No nodes have been upgraded; Grant incompatible database privileges - // to testuser before upgrading. - execSQL("CREATE USER testuser;", "", 1), - execSQL("CREATE DATABASE test;", "", 1), - execSQL("GRANT CREATE, SELECT, INSERT, UPDATE, DELETE ON DATABASE test TO testuser;", "", 1), - - showGrantsOnDatabase("system", [][]string{ - {"system", "admin", "GRANT"}, - {"system", "admin", "SELECT"}, - {"system", "root", "GRANT"}, - {"system", "root", "SELECT"}, - }), - showGrantsOnDatabase("test", [][]string{ - {"test", "admin", "ALL"}, - {"test", "root", "ALL"}, - {"test", "testuser", "CREATE"}, - {"test", "testuser", "DELETE"}, - {"test", "testuser", "INSERT"}, - {"test", "testuser", "SELECT"}, - {"test", "testuser", "UPDATE"}, - }), - - binaryUpgradeStep(c.Node(1), version21_2_4), - allowAutoUpgradeStep(1), - binaryUpgradeStep(c.Node(2), version21_2_4), - allowAutoUpgradeStep(2), - binaryUpgradeStep(c.Node(3), version21_2_4), - allowAutoUpgradeStep(3), - - waitForUpgradeStep(c.All()), - - binaryUpgradeStep(c.Node(1), mainVersion), - allowAutoUpgradeStep(1), - binaryUpgradeStep(c.Node(2), mainVersion), - allowAutoUpgradeStep(2), - binaryUpgradeStep(c.Node(3), mainVersion), - allowAutoUpgradeStep(3), - - waitForUpgradeStep(c.All()), - - showGrantsOnDatabase("system", [][]string{ - {"system", "admin", "CONNECT"}, - {"system", "root", "CONNECT"}, - }), - showGrantsOnDatabase("test", [][]string{ - {"test", "admin", "ALL"}, - {"test", "root", "ALL"}, - {"test", "testuser", "CREATE"}, - }), - - showDefaultPrivileges("test", [][]string{ - {"tables", "testuser", "SELECT"}, - {"tables", "testuser", "INSERT"}, - {"tables", "testuser", "DELETE"}, - {"tables", "testuser", "UPDATE"}, - {"types", "public", "USAGE"}, - }), - - showDefaultPrivileges("system", [][]string{ - {"types", "public", "USAGE"}, - }), - ) - u.run(ctx, t) -} - -func showGrantsOnDatabase(dbName string, expectedPrivileges [][]string) versionStep { - return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { - conn, err := u.c.ConnE(ctx, t.L(), loadNode) - require.NoError(t, err) - rows, err := conn.Query( - fmt.Sprintf("SELECT database_name, grantee, privilege_type FROM [SHOW GRANTS ON DATABASE %s]", - dbName)) - require.NoError(t, err) - var name, grantee, privilegeType string - i := 0 - for rows.Next() { - privilegeRow := expectedPrivileges[i] - err = rows.Scan(&name, &grantee, &privilegeType) - require.NoError(t, err) - - require.Equal(t, privilegeRow[0], name) - require.Equal(t, privilegeRow[1], grantee) - require.Equal(t, privilegeRow[2], privilegeType) - i++ - } - - if i != len(expectedPrivileges) { - t.Errorf("expected %d rows, found %d rows", len(expectedPrivileges), i) - } - } -} - -func showDefaultPrivileges(dbName string, expectedDefaultPrivileges [][]string) versionStep { - return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { - conn, err := u.c.ConnE(ctx, t.L(), loadNode) - require.NoError(t, err) - _, err = conn.Exec(fmt.Sprintf("USE %s", dbName)) - require.NoError(t, err) - rows, err := conn.Query("SELECT object_type, grantee, privilege_type FROM [SHOW DEFAULT PRIVILEGES FOR ALL ROLES]") - require.NoError(t, err) - - var objectType, grantee, privilegeType string - i := 0 - for rows.Next() { - defaultPrivilegeRow := expectedDefaultPrivileges[i] - err = rows.Scan(&objectType, &grantee, &privilegeType) - require.NoError(t, err) - require.Equal(t, defaultPrivilegeRow[0], objectType) - require.Equal(t, defaultPrivilegeRow[1], grantee) - require.Equal(t, defaultPrivilegeRow[2], privilegeType) - - i++ - } - - if i != len(expectedDefaultPrivileges) { - t.Errorf("expected %d rows, found %d rows", len(expectedDefaultPrivileges), i) - } - } -} diff --git a/pkg/cmd/roachtest/tests/validate_grant_option.go b/pkg/cmd/roachtest/tests/validate_grant_option.go deleted file mode 100644 index 8d1ba174ec26..000000000000 --- a/pkg/cmd/roachtest/tests/validate_grant_option.go +++ /dev/null @@ -1,184 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package tests - -import ( - "context" - - "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" - "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" - "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" - "github.com/cockroachdb/cockroach/pkg/util/version" - "github.com/stretchr/testify/require" -) - -func registerValidateGrantOption(r registry.Registry) { - r.Add(registry.TestSpec{ - Name: "validate-grant-option", - Owner: registry.OwnerSQLExperience, - Cluster: r.MakeClusterSpec(3), - Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { - runRegisterValidateGrantOption(ctx, t, c, *t.BuildVersion()) - }, - }) -} - -// execSQL executes the SQL statement as "root". -func execSQL(sqlStatement string, expectedErrText string, node int) versionStep { - return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { - conn, err := u.c.ConnE(ctx, t.L(), node) - require.NoError(t, err) - t.L().PrintfCtx(ctx, "user root on node %d executing: %s", node, sqlStatement) - _, err = conn.Exec(sqlStatement) - if len(expectedErrText) == 0 { - require.NoError(t, err) - } else { - require.EqualError(t, err, expectedErrText) - } - } -} - -// execSQLAsUser executes the SQL statement as the specified user. -func execSQLAsUser(sqlStatement string, user string, expectedErrText string, node int) versionStep { - return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { - conn, err := u.c.ConnEAsUser(ctx, t.L(), node, user) - require.NoError(t, err) - t.L().PrintfCtx(ctx, "user %s on node %d executing: %s", user, node, sqlStatement) - _, err = conn.Exec(sqlStatement) - if len(expectedErrText) == 0 { - require.NoError(t, err) - } else { - require.EqualError(t, err, expectedErrText) - } - } -} - -func runRegisterValidateGrantOption( - ctx context.Context, t test.Test, c cluster.Cluster, buildVersion version.Version, -) { - // This is meant to test a migration from 21.2 to 22.1. - if buildVersion.Major() != 22 || buildVersion.Minor() != 1 { - t.L().PrintfCtx(ctx, "skipping test because build version is %s", buildVersion) - return - } - const mainVersion = "" - predecessorVersion, err := PredecessorVersion(buildVersion) - if err != nil { - t.Fatal(err) - } - - u := newVersionUpgradeTest(c, - uploadAndStart(c.All(), predecessorVersion), - waitForUpgradeStep(c.All()), - preventAutoUpgradeStep(1), - preventAutoUpgradeStep(2), - preventAutoUpgradeStep(3), - - // No nodes have been upgraded; any user can grant or revoke a privilege that they hold (grant options - // are ignored). - execSQL("CREATE USER foo;", "", 1), - execSQL("CREATE USER foo2;", "", 2), - execSQL("CREATE USER foo3;", "", 2), - execSQL("CREATE USER foo4;", "", 2), - execSQL("CREATE USER foo5;", "", 2), - execSQL("CREATE USER target;", "", 1), - - execSQL("CREATE DATABASE d;", "", 2), - execSQL("CREATE SCHEMA s;", "", 3), - execSQL("CREATE TABLE t1();", "", 2), - execSQL("CREATE TYPE ty AS ENUM();", "", 3), - - execSQL("GRANT ALL PRIVILEGES ON DATABASE d TO foo;", "", 1), - execSQL("GRANT ALL PRIVILEGES ON SCHEMA s TO foo2;", "", 3), - execSQL("GRANT ALL PRIVILEGES ON TABLE t1 TO foo3;", "", 1), - execSQL("GRANT ALL PRIVILEGES ON TYPE ty TO foo4;", "", 1), - - execSQLAsUser("SELECT * FROM t1;", "foo3", "", 2), - execSQLAsUser("GRANT CREATE ON DATABASE d TO target;", "foo", "", 1), - execSQLAsUser("GRANT USAGE ON SCHEMA s TO target;", "foo2", "", 3), - execSQLAsUser("GRANT SELECT ON TABLE t1 TO target;", "foo3", "", 1), - execSQLAsUser("GRANT USAGE ON TYPE ty TO target;", "foo4", "", 1), - - // Node 1 is upgraded, and nodes 2 and 3 are on the previous version. - binaryUpgradeStep(c.Node(1), mainVersion), - allowAutoUpgradeStep(1), - execSQLAsUser("GRANT CREATE ON DATABASE d TO target;", "foo", "", 1), - execSQLAsUser("GRANT CREATE ON DATABASE defaultdb TO foo;", "root", "", 1), - execSQLAsUser("CREATE TABLE t2();", "foo", "", 1), // t2 created on a new node. - execSQLAsUser("CREATE TABLE t3();", "foo", "", 2), // t3 created on an old node. - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo2;", "foo", "", 1), // foo2 does not get GRANT or grant option on t2. - execSQLAsUser("GRANT GRANT, CREATE ON TABLE t3 TO foo2;", "foo", "", 2), // foo2 gets GRANT and grant option on t3 - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo", "", 1), // foo3 does not get GRANT or grant option on t2. - execSQLAsUser("GRANT GRANT, CREATE ON TABLE t3 TO foo3;", "foo", "", 2), // foo3 gets GRANT, but not grant option on t3 - - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4 WITH GRANT OPTION;", "foo", "pq: version 21.2-22 must be finalized to use grant options", 1), - execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo4 WITH GRANT OPTION;", "foo", "pq: at or near \"with\": syntax error", 2), - - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo2", - "pq: user foo2 does not have GRANT privilege on relation t2", 1), - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo2", - "pq: user foo2 does not have GRANT privilege on relation t2", 2), // Same error from node 2. - execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo3;", "foo2", "", 1), - execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo3;", "foo2", "", 2), - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4;", "foo3", - "pq: user foo3 does not have GRANT privilege on relation t2", 1), - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4;", "foo3", - "pq: user foo3 does not have GRANT privilege on relation t2", 2), // Same error from node 2. - execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo4;", "foo3", "", 1), - execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo4;", "foo3", "", 2), - - execSQLAsUser("GRANT USAGE ON SCHEMA s TO target;", "foo2", "", 3), - execSQLAsUser("GRANT INSERT ON TABLE t1 TO target;", "foo3", "", 2), - execSQLAsUser("GRANT GRANT ON TYPE ty TO target;", "foo4", "", 2), - - // Node 2 is upgraded. - binaryUpgradeStep(c.Node(2), mainVersion), - allowAutoUpgradeStep(2), - execSQLAsUser("GRANT ALL PRIVILEGES ON DATABASE d TO target;", "foo", "", 3), - execSQLAsUser("GRANT ALL PRIVILEGES ON SCHEMA s TO target;", "foo2", "", 2), - execSQLAsUser("GRANT ALL PRIVILEGES ON TABLE t1 TO target;", "foo3", "", 1), - execSQLAsUser("GRANT ALL PRIVILEGES ON TYPE ty TO target;", "foo4", "", 1), - execSQLAsUser("GRANT GRANT ON DATABASE d TO foo2;", "foo", "", 1), - execSQLAsUser("GRANT GRANT ON SCHEMA s TO foo3;", "foo2", "", 1), - execSQLAsUser("GRANT GRANT, SELECT ON TABLE t1 TO foo4;", "foo3", "", 2), - execSQLAsUser("GRANT DELETE ON TABLE t1 TO foo4;", "foo3", "", 2), - - // Node 3 is upgraded; because all the nodes have been upgraded, the migration will begin running after - // allowAutoUpgradeStep(3). The roachtest will ensure that the migration will be complete before moving - // beyond waitForUpgradeStep(c.All()). - binaryUpgradeStep(c.Node(3), mainVersion), - allowAutoUpgradeStep(3), - waitForUpgradeStep(c.All()), - - // All the nodes have been upgraded and the migration has finished; existing users will have had their grant - // option bits set equal to their privilege bits if they hold "GRANT" or "ALL" privileges. Grant options will - // now be enforced (an error will be returned if a user tries to grant a privilege they do not hold grant - // options for, which corresponds to a nonempty string in the errorExpected field). - execSQLAsUser("GRANT DELETE ON TABLE t1 TO foo2;", "foo3", "", 3), - execSQLAsUser("GRANT ALL PRIVILEGES ON SCHEMA s TO foo3;", "target", "", 2), - execSQLAsUser("GRANT CREATE ON DATABASE d TO foo3;", "foo2", "pq: user foo2 does not have CREATE privilege on database d", 2), - execSQLAsUser("GRANT USAGE ON SCHEMA s TO foo;", "foo3", "", 2), - execSQLAsUser("GRANT INSERT ON TABLE t1 TO foo;", "foo4", "pq: user foo4 does not have INSERT privilege on relation t1", 1), - execSQLAsUser("GRANT SELECT ON TABLE t1 TO foo;", "foo4", "", 1), - execSQLAsUser("GRANT DELETE ON TABLE t1 TO target;", "foo2", "pq: user foo2 missing WITH GRANT OPTION privilege on DELETE", 1), - execSQLAsUser("GRANT DELETE ON TABLE t1 TO target;", "foo4", "", 1), - execSQLAsUser("GRANT SELECT ON TABLE t1 TO target;", "foo4", "", 1), - - execSQLAsUser("GRANT SELECT ON TABLE t2 TO foo4 WITH GRANT OPTION;", "foo", "", 1), - execSQLAsUser("GRANT SELECT ON TABLE t3 TO foo4 WITH GRANT OPTION;", "foo", "", 2), - - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo2", "pq: user foo2 missing WITH GRANT OPTION privilege on CREATE", 1), - execSQLAsUser("GRANT CREATE ON TABLE t3 TO target;", "foo2", "", 2), - execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4;", "foo3", "pq: user foo3 missing WITH GRANT OPTION privilege on CREATE", 3), - execSQLAsUser("GRANT CREATE ON TABLE t3 TO target;", "foo3", "", 3), - ) - u.run(ctx, t) -} From 7c5d0e4155006fb520942568d21a63661f3a6a0e Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Mon, 23 May 2022 16:52:31 -0700 Subject: [PATCH 2/3] opt: fix performance regression due to getEnv in Optimizer.Init In f2092b6 I added a call to `randutil.NewPseudoRand` in `Optimizer.Init`. This was more expensive than I realized, because it checks the value of environment variable `COCKROACH_RANDOM_SEED` using `envutil.getEnv`, which takes a global lock. Instead, only create this rng when `testing_optimizer_random_cost_seed` is set, and seed it directly with that value to avoid the env check altogether. Fixes: #81519 Release note: None --- pkg/sql/opt/xform/BUILD.bazel | 1 - pkg/sql/opt/xform/coster.go | 7 ++++++- pkg/sql/opt/xform/optimizer.go | 4 +--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/sql/opt/xform/BUILD.bazel b/pkg/sql/opt/xform/BUILD.bazel index 70cf0cbe8810..f51af7ab55bc 100644 --- a/pkg/sql/opt/xform/BUILD.bazel +++ b/pkg/sql/opt/xform/BUILD.bazel @@ -50,7 +50,6 @@ go_library( "//pkg/util/buildutil", "//pkg/util/errorutil", "//pkg/util/log", - "//pkg/util/randutil", "//pkg/util/treeprinter", "@com_github_cockroachdb_errors//:errors", "@com_github_cockroachdb_redact//:redact", diff --git a/pkg/sql/opt/xform/coster.go b/pkg/sql/opt/xform/coster.go index 646f890aa263..c6c7a2759f5e 100644 --- a/pkg/sql/opt/xform/coster.go +++ b/pkg/sql/opt/xform/coster.go @@ -559,7 +559,12 @@ func (c *coster) ComputeCost(candidate memo.RelExpr, required *physical.Required // Don't perturb the cost if we are forcing an index. if cost < hugeCost { // Get a random value in the range [-1.0, 1.0) - multiplier := 2*c.rng.Float64() - 1 + var multiplier float64 + if c.rng == nil { + multiplier = 2*rand.Float64() - 1 + } else { + multiplier = 2*c.rng.Float64() - 1 + } // If perturbation is p, and the estimated cost of an expression is c, // the new cost is in the range [max(0, c - pc), c + pc). For example, diff --git a/pkg/sql/opt/xform/optimizer.go b/pkg/sql/opt/xform/optimizer.go index 916fa2283d82..c45c4457a752 100644 --- a/pkg/sql/opt/xform/optimizer.go +++ b/pkg/sql/opt/xform/optimizer.go @@ -24,7 +24,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/errorutil" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/cockroachdb/errors" ) @@ -127,9 +126,8 @@ func (o *Optimizer) Init(evalCtx *eval.Context, catalog cat.Catalog) { o.mem = o.f.Memo() o.explorer.init(o) seed := evalCtx.SessionData().TestingOptimizerRandomCostSeed - o.rng, _ = randutil.NewPseudoRand() if seed != 0 { - o.rng.Seed(seed) + o.rng = rand.New(rand.NewSource(seed)) // If we've been initialized with a random seed but not an explicit // perturbation value, use the max perturbation. This is used for tests // that set the seed with testing_optimizer_random_cost_seed, and will allow From 9223ca16e75925720186041fd65019f1c01918d0 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Tue, 24 May 2022 09:29:06 -0700 Subject: [PATCH 3/3] colbuilder: address a minor nit in a recent fix In 7ad29c34e02e70c0134cd42b34170ee98fa0a9b3 we made some changes about how argument to window functions are being cast to the same type in some cases. After the cast we now update the column index but forgot to update the type to the new one. However, this doesn't lead to problems since `argTypes` is only used by the aggregate window functions, and the casts can only be needed for non-aggregate window functions. Still, it's better to update the types to avoid confusion. Release note: None --- pkg/sql/colexec/colbuilder/execplan.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/sql/colexec/colbuilder/execplan.go b/pkg/sql/colexec/colbuilder/execplan.go index 056277091dff..ef15183726fc 100644 --- a/pkg/sql/colexec/colbuilder/execplan.go +++ b/pkg/sql/colexec/colbuilder/execplan.go @@ -1237,6 +1237,7 @@ func NewColOperator( } typs = append(typs, typ) argIdxs[i] = castIdx + argTypes[i] = typ } partitionColIdx := tree.NoColumnIdx