Merge #110931 #110976 #110979

110931: ui: add plan gist as option on bundle collection r=maryliag a=maryliag

Note to reviewers: There is another option to get any plan _except_ from the selected gist, but that is not part of this PR. Once a design is created, this option can be added.

---
Part Of #103018

This commit adds an option to collect statement bundle based on a specific plan gist.

<img width="578" alt="Screenshot 2023-09-19 at 3 18 01 PM" src="https://github.com/cockroachdb/cockroach/assets/1017486/5ab807b7-08f4-49e7-b540-2dadface766d">


https://www.loom.com/share/59335438f0884b75a7d163d96effe5a8

Release note (ui change): Add option to filter out by specific plan gist when collecting a statement bundle.

110976: syntheticprivilege: admin always has ALL global privileges r=rafiss a=rafiss

### syntheticprivilege: admin always has ALL global privileges

As we move away from requiring the admin role to perform cluster
debug/repair operations, we want to use a privilege instead. To
facilitate that, the admin role now implicitly has ALL global
privileges. The privilege for admins is not revokeable.

---

### sql: use better error message for missing system privilege

Since we document privileges on the GlobalPrivilegeObject using the
phrase "system privilege", we should make the error message say that
too.

informs #109814
Release note: None

110979: roachtest: use correct format directive for job ID r=yuzefovich a=yuzefovich

`catpb.JobID` doesn't implement `fmt.Stringer`.

Touches: #110782.

Epic: None

Release note: None

Co-authored-by: maryliag <marylia@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

pkg/ccl/backupccl/testdata/backup-restore/backup-permissions

@@ -46,7 +46,7 @@ REVOKE ADMIN FROM testuser
 exec-sql user=testuser
 BACKUP INTO 'userfile:///test-nonroot-cluster';
 ----
-pq: only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups: user testuser does not have BACKUP privilege on global 
+pq: only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups: user testuser does not have BACKUP system privilege
 
 # Grant system backup privilege and re-run the backup.
 exec-sql

pkg/ccl/backupccl/testdata/backup-restore/backup-permissions-deprecated

@@ -61,7 +61,7 @@ CREATE USER testuser
 exec-sql user=testuser
 BACKUP INTO 'nodelocal://1/test2'
 ----
-pq: only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups: user testuser does not have BACKUP privilege on global 
+pq: only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups: user testuser does not have BACKUP system privilege
 
 # Database backup as a non-admin user should have CONNECT on database and SELECT on tables.
 exec-sql user=testuser

pkg/ccl/backupccl/testdata/backup-restore/restore-permissions

@@ -187,7 +187,7 @@ CREATE USER testuser;
 exec-sql user=testuser
 RESTORE FROM LATEST IN 'nodelocal://1/foo';
 ----
-pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE privilege on global 
+pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE system privilege
 
 exec-sql
 GRANT SYSTEM RESTORE TO testuser;

pkg/ccl/backupccl/testdata/backup-restore/restore-permissions-deprecated

@@ -38,7 +38,7 @@ CREATE USER testuser
 exec-sql cluster=s2 user=testuser
 RESTORE FROM LATEST IN 'nodelocal://1/test/'
 ----
-pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE privilege on global 
+pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE system privilege
 
 exec-sql cluster=s2 user=testuser
 RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/test/'

pkg/ccl/backupccl/testdata/backup-restore/restore-schema-only

@@ -71,7 +71,7 @@ CREATE USER testuser
 exec-sql user=testuser
 RESTORE FROM LATEST IN 'nodelocal://1/full_cluster_backup/' with schema_only
 ----
-pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE privilege on global 
+pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE system privilege
 
 # Fail fast using a database backup
 exec-sql

pkg/ccl/backupccl/testdata/backup-restore/restore-validation-only

@@ -77,7 +77,7 @@ CREATE USER testuser
 exec-sql user=testuser
 RESTORE FROM LATEST IN 'nodelocal://1/full_cluster_backup/' with schema_only, verify_backup_table_data
 ----
-pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE privilege on global 
+pq: only users with the admin role or the RESTORE system privilege are allowed to perform a cluster restore: user testuser does not have RESTORE system privilege
 
 # Fail fast using a database backup
 exec-sql

pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges

@@ -54,7 +54,7 @@ REVOKE ADMIN FROM testuser;
 exec-sql user=testuser
 CREATE SCHEDULE foocluster FOR BACKUP INTO 'external://foo/cluster' RECURRING '@hourly';
 ----
-pq: failed to dry run backup: only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups: user testuser does not have BACKUP privilege on global 
+pq: failed to dry run backup: only users with the admin role or the BACKUP system privilege are allowed to perform full cluster backups: user testuser does not have BACKUP system privilege
 
 exec-sql user=testuser
 CREATE SCHEDULE foodb FOR BACKUP DATABASE foo INTO 'external://foo/database' RECURRING '@hourly';

pkg/ccl/streamingccl/streamproducer/replication_manager_test.go

@@ -78,12 +78,12 @@ func TestReplicationManagerRequiresReplicationPrivilege(t *testing.T) {
 		{user: "admin", expErr: "", isEnterprise: true},
 		{user: "root", expErr: "", isEnterprise: true},
 		{user: "somebody", expErr: "", isEnterprise: true},
-		{user: "nobody", expErr: "user nobody does not have REPLICATION privilege on global", isEnterprise: true},
+		{user: "nobody", expErr: "user nobody does not have REPLICATION system privilege", isEnterprise: true},
 
 		{user: "admin", expErr: "use of REPLICATION requires an enterprise license", isEnterprise: false},
 		{user: "root", expErr: " use of REPLICATION requires an enterprise license", isEnterprise: false},
 		{user: "somebody", expErr: "use of REPLICATION requires an enterprise license", isEnterprise: false},
-		{user: "nobody", expErr: "user nobody does not have REPLICATION privilege on global", isEnterprise: false},
+		{user: "nobody", expErr: "user nobody does not have REPLICATION system privilege", isEnterprise: false},
 	} {
 		t.Run(fmt.Sprintf("%s/ent=%t", tc.user, tc.isEnterprise), func(t *testing.T) {
 			if tc.isEnterprise {

pkg/cmd/roachtest/tests/jobs.go

@@ -125,7 +125,7 @@ func executeNodeShutdown(
 					t.Status("job completed")
 					return nil
 				case jobs.StatusRunning:
-					t.L().Printf("job %s still running, waiting to succeed", jobID)
+					t.L().Printf("job %d still running, waiting to succeed", jobID)
 				default:
 					// Waiting for job to complete.
 					return errors.Newf("unexpectedly found job %s in state %s", jobID, status)

pkg/sql/authorization.go

@@ -1069,6 +1069,14 @@ func insufficientPrivilegeError(
 			user, typeForError, object.GetName())
 	}
 
+	// Make a slightly different message for the global privilege object so that
+	// it uses more understandable user-facing language.
+	if object.GetObjectType() == privilege.Global {
+		return pgerror.Newf(pgcode.InsufficientPrivilege,
+			"user %s does not have %s system privilege",
+			user, kind)
+	}
+
 	return pgerror.Newf(pgcode.InsufficientPrivilege,
 		"user %s does not have %s privilege on %s %s",
 		user, kind, typeForError, object.GetName())

pkg/sql/logictest/testdata/logic_test/tenant

@@ -142,7 +142,7 @@ set default_transaction_read_only = off;
 
 user testuser
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 CREATE TENANT four
 
 subtest drop_tenant
@@ -195,13 +195,13 @@ set default_transaction_read_only = off;
 
 user testuser
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 DROP TENANT three
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 SHOW TENANTS
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 SHOW TENANT 'two'
 
 user root

pkg/sql/logictest/testdata/logic_test/tenant_builtins

@@ -264,13 +264,13 @@ SELECT crdb_internal.update_tenant_resource_limits('tenant-number-ten', 1000, 10
 
 user testuser
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 SELECT crdb_internal.create_tenant(314)
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 SELECT crdb_internal.create_tenant('not-allowed')
 
-statement error user testuser does not have MANAGETENANT privilege on global
+statement error user testuser does not have MANAGETENANT system privilege
 DROP TENANT [1]
 
 statement error only users with the admin role are allowed to gc tenant

pkg/sql/syntheticprivilege/README.md

@@ -361,7 +361,7 @@ See: `sql/logictest/testdata/logic_test/system_privileges`
 ```sql
 user testuser
 
-statement error pq: user testuser does not have MODIFYCLUSTERSETTING privilege on Global
+statement error pq: user testuser does not have MODIFYCLUSTERSETTING system privilege
 SELECT * FROM crdb_internal.cluster_settings;
 
 user root
@@ -386,7 +386,7 @@ REVOKE SYSTEM MODIFYCLUSTERSETTING FROM testuser
 
 user testuser
 
-statement error pq: user testuser does not have MODIFYCLUSTERSETTING privilege on Global
+statement error pq: user testuser does not have MODIFYCLUSTERSETTING system privilege
 SELECT * FROM crdb_internal.cluster_settings;
 
 user root

pkg/sql/syntheticprivilegecache/cache.go

@@ -171,6 +171,11 @@ func (c *Cache) readFromStorage(
 		}
 	}
 
+	// Admin always has ALL global privileges.
+	if spo.GetObjectType() == privilege.Global {
+		privDesc.Grant(username.AdminRoleName(), privilege.List{privilege.ALL}, true)
+	}
+
 	// We use InvalidID to skip checks on the root/admin roles having
 	// privileges.
 	validPrivs, err := privilege.GetValidPrivilegesForObject(spo.GetObjectType())

pkg/ui/workspaces/cluster-ui/src/api/statementDiagnosticsApi.ts

@@ -74,6 +74,7 @@ export type InsertStmtDiagnosticRequest = {
   samplingProbability?: number;
   minExecutionLatencySeconds?: number;
   expiresAfterSeconds?: number;
+  planGist: string;
 };
 
 export type InsertStmtDiagnosticResponse = {
@@ -85,8 +86,15 @@ export function createStatementDiagnosticsReport({
   samplingProbability,
   minExecutionLatencySeconds,
   expiresAfterSeconds,
+  planGist,
 }: InsertStmtDiagnosticRequest): Promise<InsertStmtDiagnosticResponse> {
   const args: any = [stmtFingerprint];
+  let query = `SELECT crdb_internal.request_statement_bundle($1, $2, $3::INTERVAL, $4::INTERVAL) as req_resp`;
+
+  if (planGist) {
+    args.push(planGist);
+    query = `SELECT crdb_internal.request_statement_bundle($1, $2, $3, $4::INTERVAL, $5::INTERVAL) as req_resp`;
+  }
 
   if (samplingProbability) {
     args.push(samplingProbability);
@@ -105,7 +113,7 @@ export function createStatementDiagnosticsReport({
   }
 
   const createStmtDiag = {
-    sql: `SELECT crdb_internal.request_statement_bundle($1, $2, $3::INTERVAL, $4::INTERVAL) as req_resp`,
+    sql: query,
     arguments: args,
   };
 

pkg/ui/workspaces/cluster-ui/src/statementDetails/diagnostics/diagnosticsView.spec.tsx

@@ -63,6 +63,7 @@ describe("DiagnosticsView", () => {
             requestTime={undefined}
             currentScale={ts}
             onChangeTimeScale={mockSetTimeScale}
+            planGists={["gist"]}
           />
         </MemoryRouter>,
       );
@@ -73,6 +74,7 @@ describe("DiagnosticsView", () => {
       activateButtonComponent.simulate("click");
       expect(activateDiagnosticsRef.current.showModalFor).toBeCalledWith(
         statementFingerprint,
+        ["gist"],
       );
     });
   });
@@ -94,6 +96,7 @@ describe("DiagnosticsView", () => {
             dismissAlertMessage={() => {}}
             currentScale={ts}
             onChangeTimeScale={mockSetTimeScale}
+            planGists={["gist"]}
           />
         </TestStoreProvider>,
       );
@@ -110,6 +113,7 @@ describe("DiagnosticsView", () => {
       activateButtonComponent.simulate("click");
       expect(activateDiagnosticsRef.current.showModalFor).toBeCalledWith(
         statementFingerprint,
+        ["gist"],
       );
     });
 
@@ -128,6 +132,7 @@ describe("DiagnosticsView", () => {
             currentScale={ts}
             requestTime={requestTime}
             onChangeTimeScale={mockSetTimeScale}
+            planGists={["gist"]}
           />
         </TestStoreProvider>,
       );
@@ -152,6 +157,7 @@ describe("DiagnosticsView", () => {
             currentScale={ts}
             requestTime={requestTime}
             onChangeTimeScale={mockSetTimeScale}
+            planGists={["gist"]}
           />
         </TestStoreProvider>,
       );

pkg/ui/workspaces/cluster-ui/src/statementDetails/diagnostics/diagnosticsView.tsx

@@ -60,6 +60,7 @@ export interface DiagnosticsViewDispatchProps {
 
 export interface DiagnosticsViewOwnProps {
   statementFingerprint?: string;
+  planGists?: string[];
 }
 
 export type DiagnosticsViewProps = DiagnosticsViewOwnProps &
@@ -80,6 +81,7 @@ const NavButton: React.FC = props => (
 
 export const EmptyDiagnosticsView = ({
   statementFingerprint,
+  planGists,
   showDiagnosticsViewLink,
   activateDiagnosticsRef,
 }: DiagnosticsViewProps): React.ReactElement => {
@@ -94,6 +96,7 @@ export const EmptyDiagnosticsView = ({
             onClick={() =>
               activateDiagnosticsRef?.current?.showModalFor(
                 statementFingerprint,
+                planGists,
               )
             }
           >
@@ -272,6 +275,7 @@ export class DiagnosticsView extends React.Component<
       activateDiagnosticsRef,
       currentScale,
       onChangeTimeScale,
+      planGists,
     } = this.props;
 
     const readyToRequestDiagnostics = diagnosticsReports.every(
@@ -329,6 +333,7 @@ export class DiagnosticsView extends React.Component<
               onClick={() =>
                 activateDiagnosticsRef?.current?.showModalFor(
                   statementFingerprint,
+                  planGists,
                 )
               }
               disabled={!readyToRequestDiagnostics}

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx

@@ -1021,6 +1021,7 @@ export class StatementDetails extends React.Component<
         onSortingChange={this.props.onSortingChange}
         currentScale={this.props.timeScale}
         onChangeTimeScale={this.changeTimeScale}
+        planGists={this.props.statementDetails.statement.stats.plan_gists}
       />
     );
   };

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.scss

@@ -53,6 +53,10 @@
     font-weight: $font-weight--light;
   }
 
+  &__plan-gist-group {
+    margin-bottom: -25px;
+  }
+
   &__conditional-container {
     display: flex;
     flex-direction: column;
@@ -73,6 +77,14 @@
     margin-bottom: $spacing-smaller;
   }
 
+  &__plan-gist-container {
+    display: flex;
+    flex-direction: row;
+    margin-top: $spacing-smaller;
+    margin-bottom: $spacing-smaller;
+    margin-left: $spacing-medium;
+  }
+
   &__trace-warning {
     font-family: $font-family--base;
     font-size: $font-size--small;
@@ -127,6 +139,15 @@
       height: 40px;
       line-height: 40px;
 
+      .ant-select-selection__rendered {
+        margin-top: 0;
+      }
+    }
+    &__plan-gist {
+      width: 400px;
+      height: 40px;
+      line-height: 40px;
+
       .ant-select-selection__rendered {
         margin-top: 0;
       }
@@ -144,3 +165,7 @@
     white-space: normal;
   }
 }
+
+.margin-bottom {
+  margin-bottom: 10px;
+}