Merge #89358 #91040 #92341 #92462 #92591 #92635 #92636 #92692

89358: memo: better selectivity estimates on single-table ORed predicates r=rytaft,michae2 a=msirek

Fixes #75090

Currently, the optimizer only does a good job of estimating the       
selectivity of single-table ORed predicates when all disjuncts are tight  
constraints. Otherwise we give up and go with the default selectivity of    
1/3 for the entire filter. If any of the disjuncts has a selectivity 
above 1/3, then the total selectivity should be at least as high as the 
selectivity of that disjunct. Any failure to find a tight constraint for
a given disjunct should only cause a selectivity of 1/3 to be used for
that disjunct, not the entire filter.  

This is addressed by using the method of combining ORed selectivities
introduced by #74303:
```
   Given predicates A and B and probability function P:

      P(A or B) = P(A) + P(B) - P(A and B)
```
The above formula is applied n-1 times, given n disjuncts.
For each disjunct which is not a tight constraint, 1/3 is used for that
predicate's selectivity in the formula.

Release note (bug fix): This patch fixes incorrect selectivity
estimation for queries with ORed predicates all referencing a
common single table.


91040: cloud/amazon: support external ID in AWS assume role r=rhu713 a=rhu713

Support passing in the optional external ID when assuming a role. This is done by extending the values of the comma-separated string value of the ASSUME_ROLE parameter to the format `<role>;external_id=<id>`. Users can still use the previous format of just `<role>` to specify a role without any external ID.

When using role chaining, each role in the chain can be associated with a different external ID. For example:
`ASSUME_ROLE=<roleA>;external_id=<idA>,<roleB>;external_id=<idB>,<roleC>` will use external ID `<idA>` to assume delegate `<roleA>`, then use external ID `<idB>` to assume delegate `<roleB>`, and finally no external ID to assume the final role `<roleC>`.

Additional documentation about external IDs can be found here: https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_create_for-user_externalid.html

Release note (enterprise change): Support passing in the optional external ID when assuming a role. This is done by extending the values of the comma-separated string value of the ASSUME_ROLE parameter to the format `<role>;external_id=<id>`. Users can still use the previous format of just `<role>` to specify a role without any external ID.

When using role chaining, each role in the chain can be associated with a different external ID. For example:
`ASSUME_ROLE=<roleA>;external_id=<idA>,<roleB>;external_id=<idB>,<roleC>` will use external ID `<idA>` to assume delegate `<roleA>`, then use external ID `<idB>` to assume delegate `<roleB>`, and finally no external ID to assume the final role `<roleC>`.

Addresses #90239

92341: changefeedccl: fix flakey tests r=[miretskiy] a=HonoreDB

Our test SQL connection doesn't always read its own writes: it can use different connections for different queries. So
```
INSERT INTO foo;
feed(CREATE CHANGEFEED FOR foo)
```
is a race condition. This just doesn't matter for most tests because if the insert happens after the initial scan, it creates the same payload. But tests with initial_scan='only' won't see it at all.

Fixes #92211

Release note: None

92462: scripts: improve pprof-loop.sh r=andrewbaptist a=tbg

Now it also works with the runtime trace endpoint (worked before but
printed concerning-looking errors) and with non-blocking endpoints
(cumulative profiles such as heap or mutex), for which it previously
busy-looped.

Epic: None
Release note: None


92591: ui: add 99.9th and 99.99th SQL latency charts r=maryliag a=maryliag

This commit introduces the charts for:
`Service Latency: SQL Statements, 99.99th percentile` and
`Service Latency: SQL Statements, 99.9th percentile` on Metrics page under SQL view.

Fixes #74247

<img width="806" alt="Screen Shot 2022-11-28 at 12 23 56 PM" src="https://user-images.githubusercontent.com/1017486/204342077-b5509f51-f94e-44be-a2e8-8bd185d12ce6.png">



Release note (ui change): Added charts
`Service Latency: SQL Statements, 99.9th percentile` and `Service Latency: SQL Statements, 99.99th percentile` to Metrics page, under SQL view.

92635: tree: fix internal error comparing tuple type with non-tuple type r=rytaft,mgartner a=msirek

Fixes #91532

This fixes comparison of an empty tuple expression, ROW() or (), or
a non-empty tuple expression with a non-tuple type by returning
an error message instead of an internal error.

Release note (bug fix): This fixes an internal error when comparing
a tuple type with a non-tuple type.

92636: backupccl: add restore corrupt descriptor validation test r=adityamaru a=msbutler

This patch adds a test that ensures that restore automatically detects corrupt restoring descriptors via the sql descriptor collection read/write machinery.

Fixes #84757

Release note: None

92692: jobs: protected timestamps refresh logic is not transactional r=fqazi a=fqazi

Fixes: #92427

Previously, the refresh logic for protected timestamps would fetch jobs with transactions before attempting to update an existing one. Due to a recent change to allow this API to reflect any changes into individual job objects, we no longer do that correctly. This patch fixes the protected timestamp manager to update timestamps in a transactionally, safe manner since multiple updates can be happening concurrently for schema changes.

Release note: None

Co-authored-by: Mark Sirek <sirek@cockroachlabs.com>
Co-authored-by: Rui Hu <rui@cockroachlabs.com>
Co-authored-by: Aaron Zinger <zinger@cockroachlabs.com>
Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Co-authored-by: maryliag <marylia@cockroachlabs.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>

pkg/ccl/backupccl/datadriven_test.go

@@ -754,6 +754,8 @@ func TestDataDriven(t *testing.T) {
 						jobutils.WaitForJobToPause(t, runner, jobID)
 					case "failed":
 						jobutils.WaitForJobToFail(t, runner, jobID)
+					case "reverting":
+						jobutils.WaitForJobReverting(t, runner, jobID)
 					default:
 						t.Fatalf("unknown state %s", state)
 					}

pkg/ccl/backupccl/testdata/backup-restore/restore-check-descriptors

@@ -0,0 +1,62 @@
+# This test checks that restore automatically detects a corrupt restoring
+# descriptor and enters the reverting state (i.e. an infinite OnFailOrCancel
+# loop with exponential back off, as Restore's OnFailOrCancel implementation
+# cannot fix this specific corrupt descriptor problem automatically) and does
+# not publish restoring database with the corrupt descriptor.
+
+new-server name=s1
+----
+
+exec-sql
+CREATE DATABASE testdb;
+CREATE TABLE testdb.parent (k INT PRIMARY KEY, v STRING);
+INSERT INTO testdb.parent (k, v) VALUES (1, 'a');
+CREATE TABLE testdb.child (k INT NOT NULL, FOREIGN KEY (k) REFERENCES testdb.parent (k))
+----
+
+exec-sql
+BACKUP INTO 'nodelocal://1/full_cluster_backup/';
+----
+
+exec-sql
+SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.before_publishing_descriptors';
+----
+
+# Rename the original testdb.child table to make it easier to invalidate the restoring child table.
+exec-sql
+ALTER TABLE testdb.child RENAME TO cool_cousin;
+----
+
+restore expect-pausepoint tag=a
+RESTORE DATABASE testdb FROM LATEST IN 'nodelocal://1/full_cluster_backup/' with new_db_name='d2'
+----
+job paused at pausepoint
+
+# Corrupt the child, the restoring table descriptor that has been written but not published
+exec-sql
+SELECT crdb_internal.unsafe_delete_descriptor(id) FROM system.namespace WHERE name = 'child'
+----
+
+exec-sql
+SELECT crdb_internal.unsafe_delete_namespace_entry("parentID", "parentSchemaID", name, id) FROM system.namespace WHERE name = 'child'
+----
+
+exec-sql
+SET CLUSTER SETTING jobs.debug.pausepoints = '';
+----
+
+
+job resume=a
+----
+
+job tag=a wait-for-state=reverting
+----
+
+query-sql regex=(prefetch descriptors: referenced descriptor ID .*: descriptor not found)
+SELECT error FROM crdb_internal.jobs WHERE job_type = 'RESTORE';
+----
+true
+
+query-sql
+SELECT * FROM [SHOW DATABASES] WHERE database_name='d2';
+----

pkg/ccl/changefeedccl/changefeed_test.go

@@ -6527,6 +6527,12 @@ func TestChangefeedOnlyInitialScan(t *testing.T) {
 				sqlDB.Exec(t, `CREATE TABLE foo (a INT PRIMARY KEY)`)
 				sqlDB.Exec(t, `INSERT INTO foo (a) SELECT * FROM generate_series(1, 5000);`)
 
+				// Most changefeed tests can afford to have a race condition between the initial
+				// inserts and starting the feed because the output looks the same for an initial
+				// scan and an insert. For tests with initial_scan=only, though, we can't start the feed
+				// until it's going to see all the initial inserts in the initial scan.
+				sqlDB.CheckQueryResultsRetry(t, `SELECT count(*) FROM foo`, [][]string{{`5000`}})
+
 				feed := feed(t, f, changefeedStmt)
 
 				sqlDB.Exec(t, "INSERT INTO foo VALUES (5005), (5007), (5009)")
@@ -6618,6 +6624,8 @@ func TestChangefeedOnlyInitialScanCSV(t *testing.T) {
 				sqlDB.Exec(t, "INSERT INTO foo VALUES (1, 'Alice'), (2, 'Bob'), (3, 'Carol')")
 				sqlDB.Exec(t, "INSERT INTO bar VALUES (1, 'a'), (2, 'b'), (3, 'c')")
 
+				sqlDB.CheckQueryResultsRetry(t, `SELECT count(*) FROM foo,bar`, [][]string{{`9`}})
+
 				feed := feed(t, f, testData.changefeedStmt)
 
 				sqlDB.Exec(t, "INSERT INTO foo VALUES (4, 'Doug'), (5, 'Elaine'), (6, 'Fred')")
@@ -6675,6 +6683,8 @@ func TestChangefeedOnlyInitialScanCSVSinkless(t *testing.T) {
 				sqlDB.Exec(t, "CREATE TABLE foo (id INT PRIMARY KEY, name STRING)")
 				sqlDB.Exec(t, "INSERT INTO foo VALUES (1, 'Alice'), (2, 'Bob'), (3, 'Carol')")
 
+				sqlDB.CheckQueryResultsRetry(t, `SELECT count(*) FROM foo`, [][]string{{`3`}})
+
 				feed := feed(t, f, changefeedStmt)
 
 				sqlDB.Exec(t, "INSERT INTO foo VALUES (4, 'Doug'), (5, 'Elaine'), (6, 'Fred')")

pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_query_plan

@@ -384,7 +384,7 @@ vectorized: true
                     │
                     └── • distinct
                         │ columns: (crdb_internal_id_shard_16, id, part)
-                        │ estimated row count: 7 (missing stats)
+                        │ estimated row count: 6 (missing stats)
                         │ distinct on: id, part
                         │
                         └── • union all
@@ -1027,7 +1027,7 @@ vectorized: true
                     │
                     └── • distinct
                         │ columns: (id, email, part)
-                        │ estimated row count: 7 (missing stats)
+                        │ estimated row count: 6 (missing stats)
                         │ distinct on: id, part
                         │
                         └── • union all

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index_query_plan

@@ -403,7 +403,7 @@ vectorized: true
                     │
                     └── • filter
                         │ columns: (crdb_internal_id_shard_16, id, crdb_region)
-                        │ estimated row count: 7 (missing stats)
+                        │ estimated row count: 6 (missing stats)
                         │ filter: (crdb_internal_id_shard_16 != 9) OR (crdb_region != 'ap-southeast-2')
                         │
                         └── • union all
@@ -1093,7 +1093,7 @@ vectorized: true
                     │
                     └── • distinct
                         │ columns: (id, email, crdb_region)
-                        │ estimated row count: 7 (missing stats)
+                        │ estimated row count: 6 (missing stats)
                         │ distinct on: id, crdb_region
                         │
                         └── • union all

pkg/cloud/amazon/aws_kms.go

@@ -43,27 +43,33 @@ func init() {
 }
 
 type kmsURIParams struct {
-	accessKey        string
-	secret           string
-	tempToken        string
-	endpoint         string
-	region           string
-	auth             string
-	roleARN          string
-	delegateRoleARNs []string
+	accessKey             string
+	secret                string
+	tempToken             string
+	endpoint              string
+	region                string
+	auth                  string
+	roleProvider          roleProvider
+	delegateRoleProviders []roleProvider
 }
 
 func resolveKMSURIParams(kmsURI cloud.ConsumeURL) (kmsURIParams, error) {
-	assumeRole, delegateRoles := cloud.ParseRoleString(kmsURI.ConsumeParam(AssumeRoleParam))
+	assumeRoleProto, delegateRoleProtos := cloud.ParseRoleProvidersString(kmsURI.ConsumeParam(AssumeRoleParam))
+	assumeRoleProvider := makeRoleProvider(assumeRoleProto)
+	delegateProviders := make([]roleProvider, len(delegateRoleProtos))
+	for i := range delegateRoleProtos {
+		delegateProviders[i] = makeRoleProvider(delegateRoleProtos[i])
+	}
+
 	params := kmsURIParams{
-		accessKey:        kmsURI.ConsumeParam(AWSAccessKeyParam),
-		secret:           kmsURI.ConsumeParam(AWSSecretParam),
-		tempToken:        kmsURI.ConsumeParam(AWSTempTokenParam),
-		endpoint:         kmsURI.ConsumeParam(AWSEndpointParam),
-		region:           kmsURI.ConsumeParam(KMSRegionParam),
-		auth:             kmsURI.ConsumeParam(cloud.AuthParam),
-		roleARN:          assumeRole,
-		delegateRoleARNs: delegateRoles,
+		accessKey:             kmsURI.ConsumeParam(AWSAccessKeyParam),
+		secret:                kmsURI.ConsumeParam(AWSSecretParam),
+		tempToken:             kmsURI.ConsumeParam(AWSTempTokenParam),
+		endpoint:              kmsURI.ConsumeParam(AWSEndpointParam),
+		region:                kmsURI.ConsumeParam(KMSRegionParam),
+		auth:                  kmsURI.ConsumeParam(cloud.AuthParam),
+		roleProvider:          assumeRoleProvider,
+		delegateRoleProviders: delegateProviders,
 	}
 
 	// Validate that all the passed in parameters are supported.
@@ -167,16 +173,16 @@ func MakeAWSKMS(ctx context.Context, uri string, env cloud.KMSEnv) (cloud.KMS, e
 		return nil, errors.Wrap(err, "new aws session")
 	}
 
-	if kmsURIParams.roleARN != "" {
+	if kmsURIParams.roleProvider != (roleProvider{}) {
 		if !env.ClusterSettings().Version.IsActive(ctx, clusterversion.V22_2SupportAssumeRoleAuth) {
 			return nil, errors.New("cannot authenticate to KMS via assume role until cluster has fully upgraded to 22.2")
 		}
 
 		// If there are delegate roles in the assume-role chain, we create a session
 		// for each role in order for it to fetch the credentials from the next role
 		// in the chain.
-		for _, role := range kmsURIParams.delegateRoleARNs {
-			intermediateCreds := stscreds.NewCredentials(sess, role)
+		for _, delegateProvider := range kmsURIParams.delegateRoleProviders {
+			intermediateCreds := stscreds.NewCredentials(sess, delegateProvider.roleARN, withExternalID(delegateProvider.externalID))
 			opts.Config.Credentials = intermediateCreds
 
 			sess, err = session.NewSessionWithOptions(opts)
@@ -185,7 +191,7 @@ func MakeAWSKMS(ctx context.Context, uri string, env cloud.KMSEnv) (cloud.KMS, e
 			}
 		}
 
-		creds := stscreds.NewCredentials(sess, kmsURIParams.roleARN)
+		creds := stscreds.NewCredentials(sess, kmsURIParams.roleProvider.roleARN, withExternalID(kmsURIParams.roleProvider.externalID))
 		opts.Config.Credentials = creds
 		sess, err = session.NewSessionWithOptions(opts)
 		if err != nil {

pkg/cloud/amazon/aws_kms_test.go

@@ -192,21 +192,32 @@ func TestEncryptDecryptAWSAssumeRole(t *testing.T) {
 		cloud.KMSEncryptDecrypt(t, uri, testEnv)
 	})
 
-	t.Run("role-chaining", func(t *testing.T) {
+	t.Run("role-chaining-external-id", func(t *testing.T) {
 		roleChainStr := os.Getenv("AWS_ROLE_ARN_CHAIN")
-		roleChain := strings.Split(roleChainStr, ",")
+		assumeRoleProvider, delegateRoleProviders := cloud.ParseRoleProvidersString(roleChainStr)
+		providerChain := append(delegateRoleProviders, assumeRoleProvider)
 
 		// First verify that none of the individual roles in the chain can be used
 		// to access the KMS.
-		for _, role := range roleChain {
-			q.Set(AssumeRoleParam, role)
+		for _, p := range providerChain {
+			q.Set(AssumeRoleParam, p.EncodeAsString())
 			roleURI := fmt.Sprintf("aws:///%s?%s", keyID, q.Encode())
 			cloud.CheckNoKMSAccess(t, roleURI, testEnv)
 		}
 
+		// Next check that the role chain without any external IDs cannot be used to
+		// access the KMS.
+		roleChainWithoutExternalID := make([]string, 0, len(providerChain))
+		for _, rp := range providerChain {
+			roleChainWithoutExternalID = append(roleChainWithoutExternalID, rp.Role)
+		}
+		q.Set(AssumeRoleParam, strings.Join(roleChainWithoutExternalID, ","))
+		uri := fmt.Sprintf("aws:///%s?%s", keyID, q.Encode())
+		cloud.CheckNoKMSAccess(t, uri, testEnv)
+
 		// Finally, check that the chain of roles can be used to access the KMS.
 		q.Set(AssumeRoleParam, roleChainStr)
-		uri := fmt.Sprintf("aws:///%s?%s", keyID, q.Encode())
+		uri = fmt.Sprintf("aws:///%s?%s", keyID, q.Encode())
 		cloud.KMSEncryptDecrypt(t, uri, testEnv)
 	})
 }