Merge #106947 #107097

106947: sql: run TestLintClusterSettingNames on CCL code r=yuzefovich a=yuzefovich

This commits enables running `TestLintClusterSettingNames` on CCL code as well as fixes some minor existing failures. I examined all of the cluster settings that are recommended to be named differently, and all of them are present on 23.1, so they have been grandfathered in.

Found while working on #76378.
Epic: None.

Release note: None

107097: changefeedccl: deflake TestChangefeedSingleColumnFamilySchemaChanges  r=miretskiy a=jayshrivastava

Previously, the test `TestChangefeedSingleColumnFamilySchemaChanges`
would flake. The reason is that the helper function, `requireErrorSoon`,
 would poll the test feed once for an error, timing out after 30 seconds.
The problem with this is that the testfeed may have an event
buffered which may be returned instead of the error. Basically, there
is a race between the buffered event an the error. To fix this,
`requireErrorSoon` should poll the test feed repeatedly for 30 seconds
for errors.

Before this change, the test would fail under stress
within 150 runs. After this change, it passes 5k runs under stress.

Fixes: #107072
Epic: None
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Jayant Shrivastava <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -5,7 +5,7 @@ admission.epoch_lifo.epoch_duration	duration	100ms	the duration of an epoch, for
 admission.epoch_lifo.queue_delay_threshold_to_switch_to_lifo	duration	105ms	the queue delay encountered by a (tenant,priority) for switching to epoch-LIFO ordering	tenant-rw
 admission.sql_kv_response.enabled	boolean	true	when true, work performed by the SQL layer when receiving a KV response is subject to admission control	tenant-rw
 admission.sql_sql_response.enabled	boolean	true	when true, work performed by the SQL layer when receiving a DistSQL response is subject to admission control	tenant-rw
-bulkio.backup.deprecated_full_backup_with_subdir.enabled	boolean	false	when true, a backup command with a user specified subdirectory will create a full backup at the subdirectory if no backup already exists at that subdirectory.	tenant-rw
+bulkio.backup.deprecated_full_backup_with_subdir.enabled	boolean	false	when true, a backup command with a user specified subdirectory will create a full backup at the subdirectory if no backup already exists at that subdirectory	tenant-rw
 bulkio.backup.file_size	byte size	128 MiB	target size for individual data files produced during BACKUP	tenant-rw
 bulkio.backup.read_timeout	duration	5m0s	amount of time after which a read attempt is considered timed out, which causes the backup to fail	tenant-rw
 bulkio.backup.read_with_priority_after	duration	1m0s	amount of time since the read-as-of time above which a BACKUP should use priority when retrying reads	tenant-rw
@@ -19,7 +19,7 @@ changefeed.fast_gzip.enabled	boolean	true	use fast gzip implementation	tenant-rw
 changefeed.node_throttle_config	string		specifies node level throttling configuration for all changefeeeds	tenant-rw
 changefeed.protect_timestamp.max_age	duration	96h0m0s	fail the changefeed if the protected timestamp age exceeds this threshold; 0 disables expiration	tenant-rw
 changefeed.schema_feed.read_with_priority_after	duration	1m0s	retry with high priority if we were not able to read descriptors for too long; 0 disables	tenant-rw
-changefeed.sink_io_workers	integer	0	the number of workers used by changefeeds when sending requests to the sink (currently webhook only): <0 disables, 0 assigns a reasonable default, >0 assigns the setting value.	tenant-rw
+changefeed.sink_io_workers	integer	0	the number of workers used by changefeeds when sending requests to the sink (currently webhook only): <0 disables, 0 assigns a reasonable default, >0 assigns the setting value	tenant-rw
 cloudstorage.azure.concurrent_upload_buffers	integer	1	controls the number of concurrent buffers that will be used by the Azure client when uploading chunks.Each buffer can buffer up to cloudstorage.write_chunk.size of memory during an upload	tenant-rw
 cloudstorage.http.custom_ca	string		custom root CA (appended to system's default CAs) for verifying certificates when interacting with HTTPS storage	tenant-rw
 cloudstorage.timeout	duration	10m0s	the timeout for import/export storage operations	tenant-rw
@@ -71,7 +71,7 @@ server.oidc_authentication.client_secret	string		sets OIDC client secret	tenant-
 server.oidc_authentication.enabled	boolean	false	enables or disabled OIDC login for the DB Console	tenant-rw
 server.oidc_authentication.principal_regex	string	(.+)	regular expression to apply to extracted principal (see claim_json_key setting) to translate to SQL user (golang regex format, must include 1 grouping to extract)	tenant-rw
 server.oidc_authentication.provider_url	string		sets OIDC provider URL ({provider_url}/.well-known/openid-configuration must resolve)	tenant-rw
-server.oidc_authentication.redirect_url	string	https://localhost:8080/oidc/v1/callback	sets OIDC redirect URL via a URL string or a JSON string containing a required `redirect_urls` key with an object that maps from region keys to URL strings (URLs should point to your load balancer and must route to the path /oidc/v1/callback) 	tenant-rw
+server.oidc_authentication.redirect_url	string	https://localhost:8080/oidc/v1/callback	sets OIDC redirect URL via a URL string or a JSON string containing a required `redirect_urls` key with an object that maps from region keys to URL strings (URLs should point to your load balancer and must route to the path /oidc/v1/callback)	tenant-rw
 server.oidc_authentication.scopes	string	openid	sets OIDC scopes to include with authentication request (space delimited list of strings, required to start with `openid`)	tenant-rw
 server.rangelog.ttl	duration	720h0m0s	if nonzero, entries in system.rangelog older than this duration are periodically purged	tenant-rw
 server.shutdown.connection_wait	duration	0s	the maximum amount of time a server waits for all SQL connections to be closed before proceeding with a drain. (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)	tenant-rw

docs/generated/settings/settings.html

@@ -11,7 +11,7 @@
 <tr><td><div id="setting-admission-kv-tenant-weights-enabled" class="anchored"><code>admission.kv.tenant_weights.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when true, tenant weights are enabled for KV admission control</td><td>Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-admission-sql-kv-response-enabled" class="anchored"><code>admission.sql_kv_response.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when true, work performed by the SQL layer when receiving a KV response is subject to admission control</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-admission-sql-sql-response-enabled" class="anchored"><code>admission.sql_sql_response.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when true, work performed by the SQL layer when receiving a DistSQL response is subject to admission control</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-bulkio-backup-deprecated-full-backup-with-subdir-enabled" class="anchored"><code>bulkio.backup.deprecated_full_backup_with_subdir.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when true, a backup command with a user specified subdirectory will create a full backup at the subdirectory if no backup already exists at that subdirectory.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-bulkio-backup-deprecated-full-backup-with-subdir-enabled" class="anchored"><code>bulkio.backup.deprecated_full_backup_with_subdir.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when true, a backup command with a user specified subdirectory will create a full backup at the subdirectory if no backup already exists at that subdirectory</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-bulkio-backup-file-size" class="anchored"><code>bulkio.backup.file_size</code></div></td><td>byte size</td><td><code>128 MiB</code></td><td>target size for individual data files produced during BACKUP</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-bulkio-backup-read-timeout" class="anchored"><code>bulkio.backup.read_timeout</code></div></td><td>duration</td><td><code>5m0s</code></td><td>amount of time after which a read attempt is considered timed out, which causes the backup to fail</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-bulkio-backup-read-with-priority-after" class="anchored"><code>bulkio.backup.read_with_priority_after</code></div></td><td>duration</td><td><code>1m0s</code></td><td>amount of time since the read-as-of time above which a BACKUP should use priority when retrying reads</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
@@ -25,7 +25,7 @@
 <tr><td><div id="setting-changefeed-node-throttle-config" class="anchored"><code>changefeed.node_throttle_config</code></div></td><td>string</td><td><code></code></td><td>specifies node level throttling configuration for all changefeeeds</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-changefeed-protect-timestamp-max-age" class="anchored"><code>changefeed.protect_timestamp.max_age</code></div></td><td>duration</td><td><code>96h0m0s</code></td><td>fail the changefeed if the protected timestamp age exceeds this threshold; 0 disables expiration</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-changefeed-schema-feed-read-with-priority-after" class="anchored"><code>changefeed.schema_feed.read_with_priority_after</code></div></td><td>duration</td><td><code>1m0s</code></td><td>retry with high priority if we were not able to read descriptors for too long; 0 disables</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-changefeed-sink-io-workers" class="anchored"><code>changefeed.sink_io_workers</code></div></td><td>integer</td><td><code>0</code></td><td>the number of workers used by changefeeds when sending requests to the sink (currently webhook only): &lt;0 disables, 0 assigns a reasonable default, &gt;0 assigns the setting value.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-changefeed-sink-io-workers" class="anchored"><code>changefeed.sink_io_workers</code></div></td><td>integer</td><td><code>0</code></td><td>the number of workers used by changefeeds when sending requests to the sink (currently webhook only): &lt;0 disables, 0 assigns a reasonable default, &gt;0 assigns the setting value</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-cloudstorage-azure-concurrent-upload-buffers" class="anchored"><code>cloudstorage.azure.concurrent_upload_buffers</code></div></td><td>integer</td><td><code>1</code></td><td>controls the number of concurrent buffers that will be used by the Azure client when uploading chunks.Each buffer can buffer up to cloudstorage.write_chunk.size of memory during an upload</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-cloudstorage-http-custom-ca" class="anchored"><code>cloudstorage.http.custom_ca</code></div></td><td>string</td><td><code></code></td><td>custom root CA (appended to system&#39;s default CAs) for verifying certificates when interacting with HTTPS storage</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-cloudstorage-timeout" class="anchored"><code>cloudstorage.timeout</code></div></td><td>duration</td><td><code>10m0s</code></td><td>the timeout for import/export storage operations</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
@@ -101,7 +101,7 @@
 <tr><td><div id="setting-server-oidc-authentication-enabled" class="anchored"><code>server.oidc_authentication.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables or disabled OIDC login for the DB Console</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-oidc-authentication-principal-regex" class="anchored"><code>server.oidc_authentication.principal_regex</code></div></td><td>string</td><td><code>(.+)</code></td><td>regular expression to apply to extracted principal (see claim_json_key setting) to translate to SQL user (golang regex format, must include 1 grouping to extract)</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-oidc-authentication-provider-url" class="anchored"><code>server.oidc_authentication.provider_url</code></div></td><td>string</td><td><code></code></td><td>sets OIDC provider URL ({provider_url}/.well-known/openid-configuration must resolve)</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-server-oidc-authentication-redirect-url" class="anchored"><code>server.oidc_authentication.redirect_url</code></div></td><td>string</td><td><code>https://localhost:8080/oidc/v1/callback</code></td><td>sets OIDC redirect URL via a URL string or a JSON string containing a required `redirect_urls` key with an object that maps from region keys to URL strings (URLs should point to your load balancer and must route to the path /oidc/v1/callback) </td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-server-oidc-authentication-redirect-url" class="anchored"><code>server.oidc_authentication.redirect_url</code></div></td><td>string</td><td><code>https://localhost:8080/oidc/v1/callback</code></td><td>sets OIDC redirect URL via a URL string or a JSON string containing a required `redirect_urls` key with an object that maps from region keys to URL strings (URLs should point to your load balancer and must route to the path /oidc/v1/callback)</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-oidc-authentication-scopes" class="anchored"><code>server.oidc_authentication.scopes</code></div></td><td>string</td><td><code>openid</code></td><td>sets OIDC scopes to include with authentication request (space delimited list of strings, required to start with `openid`)</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-rangelog-ttl" class="anchored"><code>server.rangelog.ttl</code></div></td><td>duration</td><td><code>720h0m0s</code></td><td>if nonzero, entries in system.rangelog older than this duration are periodically purged</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-secondary-tenants-redact-trace-enabled" class="anchored"><code>server.secondary_tenants.redact_trace.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>controls if server side traces are redacted for tenant operations</td><td>Dedicated/Self-Hosted</td></tr>

docs/tech-notes/version_upgrades.md

@@ -208,8 +208,6 @@ Here are the remaining uses:
 - auto-generate a random UUID for `cluster.secret`.
 - block the node startup if a user/role with name `public` is present in `system.users`.
 - create the `defaultdb` and `postgres` empty databases.
-- copy the values from the old `timeseries.storage.10s_resolution_ttl` and `timeseries.storage.30m_resolution_ttl` settings
-  to their new setting names.
 - add the default lat/long entries to `system.locations`.
 - add the `CREATELOGIN` option to roles that already have the `CREATEROLE` role option.
 

pkg/BUILD.bazel

@@ -556,6 +556,7 @@ ALL_TESTS = [
     "//pkg/sql/sqlitelogictest/tests/local:local_test",
     "//pkg/sql/sqlliveness/slinstance:slinstance_test",
     "//pkg/sql/sqlliveness/slstorage:slstorage_test",
+    "//pkg/sql/sqlnoccltest:sqlnoccltest_test",
     "//pkg/sql/sqlstats/insights/integration:integration_test",
     "//pkg/sql/sqlstats/insights:insights_test",
     "//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil:sqlstatsutil_test",
@@ -2052,6 +2053,7 @@ GO_TARGETS = [
     "//pkg/sql/sqlliveness/slstorage:slstorage_test",
     "//pkg/sql/sqlliveness/sqllivenesstestutils:sqllivenesstestutils",
     "//pkg/sql/sqlliveness:sqlliveness",
+    "//pkg/sql/sqlnoccltest:sqlnoccltest_test",
     "//pkg/sql/sqlstats/insights/integration:integration_test",
     "//pkg/sql/sqlstats/insights:insights",
     "//pkg/sql/sqlstats/insights:insights_test",

pkg/ccl/backupccl/backupdest/backup_destination.go

@@ -62,7 +62,7 @@ var featureFullBackupUserSubdir = settings.RegisterBoolSetting(
 	settings.TenantWritable,
 	"bulkio.backup.deprecated_full_backup_with_subdir.enabled",
 	"when true, a backup command with a user specified subdirectory will create a full backup at"+
-		" the subdirectory if no backup already exists at that subdirectory.",
+		" the subdirectory if no backup already exists at that subdirectory",
 	false,
 ).WithPublic()
 

pkg/ccl/changefeedccl/changefeed_test.go

@@ -3139,22 +3139,28 @@ func TestChangefeedOutputTopics(t *testing.T) {
 	cdcTest(t, testFn, feedTestForceSink("kafka"))
 }
 
+// requireErrorSoon polls for the test feed for an error and asserts that
+// the error matches the provided regex.
 func requireErrorSoon(
 	ctx context.Context, t *testing.T, f cdctest.TestFeed, errRegex *regexp.Regexp,
 ) {
-	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
-	defer cancel()
-	done := make(chan struct{})
-	go func() {
-		if _, err := f.Next(); err != nil {
-			assert.Regexp(t, errRegex, err)
-			done <- struct{}{}
+	err := timeutil.RunWithTimeout(ctx, "requireErrorSoon", 30*time.Second, func(ctx context.Context) error {
+		for {
+			select {
+			case <-ctx.Done():
+				return ctx.Err()
+			default:
+				m, err := f.Next()
+				if err != nil {
+					assert.Regexp(t, errRegex, err)
+					return nil
+				}
+				log.Infof(ctx, "waiting for error; skipping test feed message: %s", m.String())
+			}
 		}
-	}()
-	select {
-	case <-ctx.Done():
-		t.Fatal("timed out waiting for changefeed to fail")
-	case <-done:
+	})
+	if err != nil {
+		t.Fatal(err)
 	}
 }
 

pkg/ccl/changefeedccl/changefeedbase/settings.go

@@ -74,7 +74,7 @@ var IdleTimeout = settings.RegisterDurationSetting(
 var FrontierCheckpointFrequency = settings.RegisterDurationSetting(
 	settings.TenantWritable,
 	"changefeed.frontier_checkpoint_frequency",
-	"controls the frequency with which span level checkpoints will be written; if 0, disabled.",
+	"controls the frequency with which span level checkpoints will be written; if 0, disabled",
 	10*time.Minute,
 	settings.NonNegativeDuration,
 )
@@ -297,7 +297,7 @@ var SinkIOWorkers = settings.RegisterIntSetting(
 	settings.TenantWritable,
 	"changefeed.sink_io_workers",
 	"the number of workers used by changefeeds when sending requests to the sink "+
-		"(currently webhook only): <0 disables, 0 assigns a reasonable default, >0 assigns the setting value.",
+		"(currently webhook only): <0 disables, 0 assigns a reasonable default, >0 assigns the setting value",
 	0,
 ).WithPublic()
 

pkg/ccl/oidcccl/settings.go

@@ -182,7 +182,7 @@ var OIDCRedirectURL = func() *settings.StringSetting {
 		OIDCRedirectURLSettingName,
 		"sets OIDC redirect URL via a URL string or a JSON string containing a required "+
 			"`redirect_urls` key with an object that maps from region keys to URL strings "+
-			"(URLs should point to your load balancer and must route to the path /oidc/v1/callback) ",
+			"(URLs should point to your load balancer and must route to the path /oidc/v1/callback)",
 		"https://localhost:8080/oidc/v1/callback",
 		validateOIDCRedirectURL,
 	)

pkg/ccl/streamingccl/settings.go

@@ -58,7 +58,7 @@ var StreamReplicationMinCheckpointFrequency = settings.RegisterDurationSetting(
 	settings.SystemOnly,
 	"stream_replication.min_checkpoint_frequency",
 	"controls minimum frequency the stream replication source cluster sends checkpoints "+
-		"to the destination cluster.",
+		"to the destination cluster",
 	10*time.Second,
 	settings.NonNegativeDuration,
 )

pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go

@@ -41,7 +41,7 @@ import (
 var JobCheckpointFrequency = settings.RegisterDurationSetting(
 	settings.TenantWritable,
 	"stream_replication.job_checkpoint_frequency",
-	"controls the frequency with which partitions update their progress; if 0, disabled.",
+	"controls the frequency with which partitions update their progress; if 0, disabled",
 	10*time.Second,
 	settings.NonNegativeDuration,
 )

pkg/settings/registry.go

@@ -172,6 +172,7 @@ var retiredSettings = map[string]struct{}{
 	"changefeed.replan_flow_frequency":                         {},
 	"changefeed.replan_flow_threshold":                         {},
 	"jobs.trace.force_dump_mode":                               {},
+	"timeseries.storage.30m_resolution_ttl":                    {},
 }
 
 // sqlDefaultSettings is the list of "grandfathered" existing sql.defaults

pkg/sql/BUILD.bazel

@@ -658,7 +658,6 @@ go_test(
         "mutation_test.go",
         "mvcc_backfiller_test.go",
         "normalization_test.go",
-        "partition_test.go",
         "pg_metadata_test.go",
         "pg_oid_test.go",
         "pgwire_internal_test.go",
@@ -729,6 +728,7 @@ go_test(
     deps = [
         "//pkg/base",
         "//pkg/build/bazel",
+        "//pkg/ccl",
         "//pkg/cloud/impl:cloudimpl",
         "//pkg/clusterversion",
         "//pkg/col/coldata",