Merge #149480 #149485 #149606

149480: opt/optgen: refactor NormalizeLikeAny and compile string and integer literals as Go values r=mgartner a=mgartner

#### opt: refactor NormalizeLikeAny

The NormalizeLikeAny rule no longer allocates, memoizes, and interns a
constant string expression for the literal "%" value.

Release note: None

#### opt/optgen: compile string and integer literals as Go values

Previously, optgen would sometimes compile string and integer literals,
e.g., `"foo"` and `1`, as Go strings and integers, and other times as
`*tree.DString`s and `*tree.DInt`s, depending on the context. Now, these
literals are always compiled as Go values. In addition to helping
prevent confusion, this can help avoid unnecessary allocations for
datums when they are not needed, e.g., see `NormalizeLikeAny`.

Release note: None


149485: changefeedccl: add CHANGEFEED privilege to DB and Schema privileges r=rafiss a=KeithCh

add CHANGEFEED privilege to DB and Schema privileges to support 
database/schema-level changefeeds.

Resolves: #149470

Release note: None

149606: roachtest: fail on bad table lookup when fingerprinting fixtures r=msbutler a=kev-cao

Some fixtures are missing table fingerprints, which results in failing restore roachtests. However, the failure is actually caused by bad fixture fingerprints, which were silently ignored due to the fact that we simply log an error and continue if we are unable to fetch all tables of a database. This commit updates the roachtest to properly fail if we fail to fetch a table.

Informs: #149555, #149556, #149557, #149558, #149559, #149560, #149561

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Keith Chow <[email protected]>
Co-authored-by: Kevin Cao <[email protected]>

Author: 4 people

pkg/backup/show_test.go

@@ -371,7 +371,7 @@ GRANT UPDATE ON top_secret TO agent_bond;
 				`GRANT CREATE, USAGE ON SCHEMA public TO public; ` +
 				`GRANT ALL ON SCHEMA public TO root WITH GRANT OPTION; `, `root`},
 			{`locator`, `schema`, `GRANT ALL ON SCHEMA locator TO admin WITH GRANT OPTION; ` +
-				`GRANT CREATE ON SCHEMA locator TO agent_bond; ` +
+				`GRANT CHANGEFEED, CREATE ON SCHEMA locator TO agent_bond; ` +
 				`GRANT ALL ON SCHEMA locator TO m; ` +
 				`GRANT ALL ON SCHEMA locator TO root WITH GRANT OPTION; `, `root`},
 			{`continent`, `type`, `GRANT ALL ON TYPE continent TO admin WITH GRANT OPTION; ` +

pkg/ccl/changefeedccl/changefeed_test.go

@@ -3707,9 +3707,6 @@ func TestChangefeedGrant(t *testing.T) {
 		rootDB := sqlutils.MakeSQLRunner(s.DB)
 		rootDB.Exec(t, `create user guest`)
 
-		// GRANT CHANGEFEED ON DATABASE is an error.
-		rootDB.ExpectErr(t, `invalid privilege type CHANGEFEED for database`, `GRANT CHANGEFEED ON DATABASE d TO guest`)
-
 		// CHANGEFEED can be granted as a default privilege on all new tables in a schema
 		rootDB.ExecMultiple(t,
 			`ALTER DEFAULT PRIVILEGES IN SCHEMA d.public GRANT CHANGEFEED ON TABLES TO guest`,

pkg/cmd/roachtest/tests/backup_fixtures.go

@@ -415,8 +415,7 @@ func (bd *backupDriver) fingerprintFixture(ctx context.Context) map[string]strin
 	conn := bd.c.Conn(ctx, bd.t.L(), 1)
 	defer conn.Close()
 	return fingerprintDatabase(
-		ctx, bd.t, conn,
-		bd.sp.fixture.DatabaseName(), bd.getLatestAOST(sqlutils.MakeSQLRunner(conn)),
+		bd.t, conn, bd.sp.fixture.DatabaseName(), bd.getLatestAOST(sqlutils.MakeSQLRunner(conn)),
 	)
 }
 
@@ -440,10 +439,10 @@ func (bd *backupDriver) getLatestAOST(sql *sqlutils.SQLRunner) string {
 // and returns a map of fully qualified table names to their fingerprints.
 // If AOST is not provided, the current time is used as the AOST.
 func fingerprintDatabase(
-	ctx context.Context, t test.Test, conn *gosql.DB, dbName string, aost string,
+	t test.Test, conn *gosql.DB, dbName string, aost string,
 ) map[string]string {
 	sql := sqlutils.MakeSQLRunner(conn)
-	tables := getDatabaseTables(ctx, t, sql, dbName)
+	tables := getDatabaseTables(t, sql, dbName)
 	if len(tables) == 0 {
 		t.L().Printf("no tables found in database %s", dbName)
 		return nil
@@ -480,9 +479,7 @@ func fingerprintDatabase(
 // fixture.
 // Note: This assumes there aren't any funky characters in the identifiers, so
 // nothing is SQL-escaped.
-func getDatabaseTables(
-	ctx context.Context, t test.Test, sql *sqlutils.SQLRunner, db string,
-) []string {
+func getDatabaseTables(t test.Test, sql *sqlutils.SQLRunner, db string) []string {
 	tablesQuery := fmt.Sprintf(`SELECT schema_name, table_name FROM [SHOW TABLES FROM %s]`, db)
 	rows := sql.Query(t, tablesQuery)
 	defer rows.Close()
@@ -491,8 +488,7 @@ func getDatabaseTables(
 	for rows.Next() {
 		var schemaName, tableName string
 		if err := rows.Scan(&schemaName, &tableName); err != nil {
-			t.L().Printf("error scanning table name: %v", err)
-			continue
+			require.NoError(t, err, "error scanning table name")
 		}
 		tables = append(tables, fmt.Sprintf(`%s.%s.%s`, db, schemaName, tableName))
 	}

pkg/cmd/roachtest/tests/restore.go

@@ -931,7 +931,7 @@ func (rd *restoreDriver) maybeValidateFingerprint(ctx context.Context) {
 	require.NoError(rd.t, err)
 	defer conn.Close()
 	fingerprint := fingerprintDatabase(
-		ctx, rd.t, conn, rd.sp.backup.fixture.DatabaseName(), "", /* aost */
+		rd.t, conn, rd.sp.backup.fixture.DatabaseName(), "", /* aost */
 	)
 	require.Equal(rd.t, rd.fixtureMetadata.Fingerprint, fingerprint, "fingerprint mismatch after restore")
 }

pkg/sql/catalog/catpb/privilege_test.go

@@ -138,12 +138,12 @@ func TestPrivilege(t *testing.T) {
 			},
 			privilege.Table,
 		},
-		// Ensure revoking BACKUP, CONNECT, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG, RESTORE
+		// Ensure revoking BACKUP, CONNECT, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG, RESTORE, CHANGEFEED
 		// from a user with ALL privilege on a database leaves the user with no privileges.
 		{testUser,
 			privilege.List{privilege.ALL},
 			privilege.List{privilege.BACKUP, privilege.CONNECT, privilege.CREATE, privilege.DROP, privilege.SELECT,
-				privilege.INSERT, privilege.DELETE, privilege.UPDATE, privilege.ZONECONFIG, privilege.RESTORE},
+				privilege.INSERT, privilege.DELETE, privilege.UPDATE, privilege.ZONECONFIG, privilege.RESTORE, privilege.CHANGEFEED},
 			[]catpb.UserPrivilege{
 				{User: username.AdminRoleName(), Privileges: []privilege.Privilege{{Kind: privilege.ALL, GrantOption: true}}},
 			},
@@ -555,6 +555,16 @@ func TestGrantWithGrantOption(t *testing.T) {
 			privilege.List{privilege.ALL, privilege.CREATE},
 			privilege.List{privilege.ALL},
 			privilege.List{privilege.ALL}},
+		{catpb.NewPrivilegeDescriptor(testUser, privilege.List{}, privilege.List{}, username.AdminRoleName()),
+			testUser, privilege.Schema,
+			privilege.List{privilege.CHANGEFEED},
+			privilege.List{privilege.CHANGEFEED},
+			privilege.List{privilege.CHANGEFEED}},
+		{catpb.NewPrivilegeDescriptor(testUser, privilege.List{}, privilege.List{}, username.AdminRoleName()),
+			testUser, privilege.Database,
+			privilege.List{privilege.CHANGEFEED},
+			privilege.List{privilege.CHANGEFEED},
+			privilege.List{privilege.CHANGEFEED}},
 	}
 
 	for tcNum, tc := range testCases {
@@ -651,6 +661,20 @@ func TestRevokeWithGrantOption(t *testing.T) {
 			privilege.List{},
 			privilege.List{},
 			true},
+		{catpb.NewPrivilegeDescriptor(testUser, privilege.List{privilege.CHANGEFEED}, privilege.List{privilege.CHANGEFEED}, username.AdminRoleName()),
+			testUser, privilege.Database,
+			false,
+			privilege.List{privilege.CHANGEFEED},
+			privilege.List{},
+			privilege.List{},
+			true},
+		{catpb.NewPrivilegeDescriptor(testUser, privilege.List{privilege.CHANGEFEED}, privilege.List{privilege.CHANGEFEED}, username.AdminRoleName()),
+			testUser, privilege.Schema,
+			false,
+			privilege.List{privilege.CHANGEFEED},
+			privilege.List{},
+			privilege.List{},
+			true},
 	}
 
 	for tcNum, tc := range testCases {

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_schema

@@ -84,7 +84,9 @@ database_name  schema_name  grantee    privilege_type  is_grantable
 test           s2           admin      ALL             true
 test           s2           root       ALL             true
 test           s2           testuser   CREATE          false
+test           s2           testuser   CHANGEFEED      false
 test           s2           testuser2  CREATE          false
+test           s2           testuser2  CHANGEFEED      false
 
 statement ok
 ALTER DEFAULT PRIVILEGES REVOKE ALL ON SCHEMAS FROM testuser, testuser2

pkg/sql/logictest/testdata/logic_test/grant_database

@@ -58,12 +58,14 @@ SHOW GRANTS ON DATABASE a
 a  admin      ALL         true
 a  public     CONNECT     false
 a  readwrite  BACKUP      true
+a  readwrite  CHANGEFEED  true
 a  readwrite  CREATE      true
 a  readwrite  DROP        true
 a  readwrite  RESTORE     true
 a  readwrite  ZONECONFIG  true
 a  root       ALL         true
 a  test-user  BACKUP      true
+a  test-user  CHANGEFEED  true
 a  test-user  CREATE      true
 a  test-user  DROP        true
 a  test-user  RESTORE     true
@@ -74,11 +76,13 @@ SHOW GRANTS ON DATABASE a FOR readwrite, "test-user"
 ----
 a  public     CONNECT     false
 a  readwrite  BACKUP      true
+a  readwrite  CHANGEFEED  true
 a  readwrite  CREATE      true
 a  readwrite  DROP        true
 a  readwrite  RESTORE     true
 a  readwrite  ZONECONFIG  true
 a  test-user  BACKUP      true
+a  test-user  CHANGEFEED  true
 a  test-user  CREATE      true
 a  test-user  DROP        true
 a  test-user  RESTORE     true
@@ -93,12 +97,14 @@ SHOW GRANTS ON DATABASE a
 a  admin      ALL         true
 a  public     CONNECT     false
 a  readwrite  BACKUP      true
+a  readwrite  CHANGEFEED  true
 a  readwrite  CREATE      true
 a  readwrite  DROP        true
 a  readwrite  RESTORE     true
 a  readwrite  ZONECONFIG  true
 a  root       ALL         true
 a  test-user  BACKUP      true
+a  test-user  CHANGEFEED  true
 a  test-user  DROP        true
 a  test-user  RESTORE     true
 a  test-user  ZONECONFIG  true
@@ -111,6 +117,7 @@ SHOW GRANTS ON DATABASE a FOR readwrite, "test-user"
 ----
 a  public     CONNECT     false
 a  readwrite  BACKUP      true
+a  readwrite  CHANGEFEED  true
 a  readwrite  CREATE      true
 a  readwrite  DROP        true
 a  readwrite  RESTORE     true

pkg/sql/opt/norm/general_funcs.go

@@ -1540,16 +1540,16 @@ func (c *CustomFuncs) CanAddConstInts(first tree.Datum, second tree.Datum) bool
 	return ok
 }
 
+// DInt returns a new *tree.DInt with the given integer value.
+func (c *CustomFuncs) DInt(i tree.DInt) *tree.DInt {
+	return tree.NewDInt(i)
+}
+
 // IntConst constructs a Const holding a DInt.
 func (c *CustomFuncs) IntConst(d *tree.DInt) opt.ScalarExpr {
 	return c.f.ConstructConst(d, types.Int)
 }
 
-// StrConst constructs a Const holding a DString.
-func (c *CustomFuncs) StrConst(d *tree.DString) opt.ScalarExpr {
-	return c.f.ConstructConst(d, types.String)
-}
-
 // StringFromConst extracts a string from a Const expression. It returns the
 // string and a boolean indicating whether the extraction was successful.
 func (c *CustomFuncs) StringFromConst(expr opt.ScalarExpr) (string, bool) {
@@ -1565,14 +1565,13 @@ func (c *CustomFuncs) StringFromConst(expr opt.ScalarExpr) (string, bool) {
 	return "", false
 }
 
-// EqualConstString compares two Const expressions to see if they hold equal string values.
-func (c *CustomFuncs) EqualConstStrings(left, right opt.ScalarExpr) bool {
-	leftStr, okLeft := c.StringFromConst(left)
-	rightStr, okRight := c.StringFromConst(right)
-	if !okLeft || !okRight {
-		return false
+// ConstStringEquals returns true if e is a constant string expression and is
+// equal to other.
+func (c *CustomFuncs) ConstStringEquals(e opt.ScalarExpr, other string) bool {
+	if eStr, ok := c.StringFromConst(e); ok {
+		return eStr == other
 	}
-	return leftStr == rightStr
+	return false
 }
 
 // IsGreaterThan returns true if the first datum compares as greater than the

pkg/sql/opt/norm/rules/decorrelate.opt

@@ -707,7 +707,7 @@
     $left:*
     $right:(Limit $input:* (Const $limit:*) $ordering:*) &
         (HasOuterCols $right) &
-        (IsGreaterThan $limit 1)
+        (IsGreaterThan $limit (DInt 1))
     $on:*
     $private:*
 )

pkg/sql/opt/norm/rules/groupby.opt

@@ -324,7 +324,7 @@
 (ConstructProjectionFromDistinctOn
     (Limit
         $input
-        (IntConst 1)
+        (IntConst (DInt 1))
         (GroupingInputOrdering $groupingPrivate)
     )
     (MakeEmptyColSet)
@@ -610,7 +610,7 @@
 #      a. The aggregate only references cols from the Window operator's input.
 #      b. The aggregate is a ConstAgg (or ConstNotNull, AnyNotNull, or FirstAgg)
 #         that passes through the result of a window function.
-# 
+#
 # Assuming all of the above are satisfied, each GroupBy aggregate that only
 # references the Window's input can be left alone (5a). Then, each ConstAgg
 # referencing a window function can be replaced by that function (5b).