Merge #97728 #101869 #102209 #102297

97728: lease: Fixed a small if-condition for AcquireFreshestFromStore r=Xiang-Gu a=Xiang-Gu

Previously, the condition is `if xx > 1` while it's enough to do `if xx > 0`.

Epic: None

101869: sql: implement DESCENDING order PKs for row-level TTL r=rafiss a=ecwall

Fixes #76912

Updates the schema change validation to allow TTL tables to have DESC PK columns. The functionality is version gated behind `V23_2TTLAllowDescPK`.

Changes the TTL query bounds `WHERE` clause from this format:
```
AND (col1, col2) >= ($4, $5) AND (col1, col2) < ($2, $3)
```
to this format:
```
AND (
  (col1 > $4) OR
  (col1 = $4 AND col2 >= $5)
)
AND (
  (col1 < $2) OR
  (col1 = $2 AND col2 < $3)
)

```
which allows for DESC order keys:
```
AND (
  (col1 < $4) OR
  (col1 = $4 AND col2 <= $5)
)
AND (
  (col1 > $2) OR
  (col1 = $2 AND col2 > $3)
)
```
or mixed ASC/DESC order keys:
```
AND (
  (col1 > $4) OR
  (col1 = $4 AND col2 <= $5)
)
AND (
  (col1 < $2) OR
  (col1 = $2 AND col2 > $3)
)
```

The `ORDER BY` clause also replaces implicit ascending ordering:
```
ORDER BY col1, col2
```
with explicit ordering:
```
ORDER BY col1 ASC, col2 DESC
```

Release note (sql change): TTL supports DESC order primay key columns.

Depends on
~#102030
~#102127

102209: sqlstats: improve `GetPercentileValues` performance r=maryliag a=maryliag

Previously, the function to Query the percentile values
was also doing a flush of the stream. This was required
because during Insights detector, everytime some data was
added a query was done after, so a flush had to be done to
guarantee precision.
When making the usage of the same Query function for the
`GetPercentileValues` it was still doing a flush, but that
was not necessary, since there was no new data, and if there
was the detector was going to take care of the flush. It could
also be causing contention on the stream.

Since there is no need for the flush when retrieving the info
for sql stats, this commits updates the Query function
to have an optional flush, that can be skipped on that case.
Probably other improvements can be made on the existing path,
but this PR focus on the performance degradations caused by
the function `GetPercentileValues` and no behaviour change
is made on other existing flows.

Fixes #102208

Before
<img width="1218" alt="current-total" src="https://user-images.githubusercontent.com/1017486/234451031-d37a85c0-b322-497b-8d0c-a8851a8cffe8.png">


After
<img width="870" alt="op-flush-total" src="https://user-images.githubusercontent.com/1017486/234451041-5b9104a9-8663-4322-a2a2-f7a979bfa76b.png">

---

Before
<img width="875" alt="current-pink" src="https://user-images.githubusercontent.com/1017486/234451225-0b552281-d162-4e64-9d37-80bec61e4810.png">


After
<img width="871" alt="op-flush-pink" src="https://user-images.githubusercontent.com/1017486/234451243-6838cc19-aeaf-4f26-9a2f-7c391f6aba79.png">

---

Before
<img width="868" alt="current" src="https://user-images.githubusercontent.com/1017486/234451261-a12736ec-baec-44ef-9004-884c136b484b.png">

After
<img width="808" alt="op-flush" src="https://user-images.githubusercontent.com/1017486/234451278-85178109-65f5-4ad0-82a2-2ecb3e786103.png">

---

Before
<img width="1613" alt="current-query" src="https://user-images.githubusercontent.com/1017486/234451301-e59241d8-a089-440b-9a23-3d36ba5413d7.png">

After
<img width="924" alt="op-flush-query" src="https://user-images.githubusercontent.com/1017486/234451327-19d95775-0443-4f81-a4c7-d3a6c3253245.png">

Release note: None

102297: flowinfra: remove stale TODO r=yuzefovich a=yuzefovich

I don't think the code movement proposed in the TODO makes sense anymore.

Epic: None

Release note: None

Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

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

@@ -294,4 +294,4 @@ trace.opentelemetry.collector	string		address of an OpenTelemetry trace collecto
 trace.snapshot.rate	duration	0s	if non-zero, interval at which background trace snapshots are captured	tenant-rw
 trace.span_registry.enabled	boolean	true	if set, ongoing traces can be seen at https://<ui>/#/debug/tracez	tenant-rw
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.	tenant-rw
-version	version	1000023.1-2	set the active cluster version in the format '<major>.<minor>'	tenant-rw
+version	version	1000023.1-4	set the active cluster version in the format '<major>.<minor>'	tenant-rw

docs/generated/settings/settings.html

@@ -247,6 +247,6 @@
 <tr><td><div id="setting-trace-snapshot-rate" class="anchored"><code>trace.snapshot.rate</code></div></td><td>duration</td><td><code>0s</code></td><td>if non-zero, interval at which background trace snapshots are captured</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-trace-span-registry-enabled" class="anchored"><code>trace.span_registry.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://&lt;ui&gt;/#/debug/tracez</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-trace-zipkin-collector" class="anchored"><code>trace.zipkin.collector</code></div></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as &lt;host&gt;:&lt;port&gt;. If no port is specified, 9411 will be used.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.1-2</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.1-4</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 </tbody>
 </table>

pkg/BUILD.bazel

@@ -553,6 +553,7 @@ ALL_TESTS = [
     "//pkg/sql/stmtdiagnostics:stmtdiagnostics_test",
     "//pkg/sql/syntheticprivilege:syntheticprivilege_test",
     "//pkg/sql/tests:tests_test",
+    "//pkg/sql/ttl/ttlbase:ttlbase_test",
     "//pkg/sql/ttl/ttljob:ttljob_test",
     "//pkg/sql/types:types_disallowed_imports_test",
     "//pkg/sql/types:types_test",
@@ -2010,6 +2011,7 @@ GO_TARGETS = [
     "//pkg/sql/tests:tests",
     "//pkg/sql/tests:tests_test",
     "//pkg/sql/ttl/ttlbase:ttlbase",
+    "//pkg/sql/ttl/ttlbase:ttlbase_test",
     "//pkg/sql/ttl/ttljob:ttljob",
     "//pkg/sql/ttl/ttljob:ttljob_test",
     "//pkg/sql/ttl/ttlschedule:ttlschedule",

pkg/clusterversion/cockroach_versions.go

@@ -542,6 +542,10 @@ const (
 	// Step 1b: Add new version for 23.2 development here.
 	// Do not add new versions to a patch release.
 	// *************************************************
+
+	// V23_2TTLAllowDescPK is the version where TTL tables can have descending
+	// primary keys.
+	V23_2TTLAllowDescPK
 )
 
 func (k Key) String() string {
@@ -941,6 +945,10 @@ var rawVersionsSingleton = keyedVersions{
 	// Step 2b: Add new version gates for 23.2 development here.
 	// Do not add new versions to a patch release.
 	// *************************************************
+	{
+		Key:     V23_2TTLAllowDescPK,
+		Version: roachpb.Version{Major: 23, Minor: 1, Internal: 4},
+	},
 }
 
 // developmentBranch must true on the main development branch but

pkg/sql/alter_table.go

@@ -25,7 +25,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver"
@@ -495,7 +494,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 			}
 
 			tableDesc := n.tableDesc
-			if t.Column == colinfo.TTLDefaultExpirationColumnName &&
+			if t.Column == catpb.TTLDefaultExpirationColumnName &&
 				tableDesc.HasRowLevelTTL() &&
 				tableDesc.GetRowLevelTTL().HasDurationExpr() {
 				return errors.WithHintf(
@@ -607,7 +606,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 					"column %q in the middle of being dropped", t.GetColumn())
 			}
 			columnName := col.GetName()
-			if columnName == colinfo.TTLDefaultExpirationColumnName &&
+			if columnName == catpb.TTLDefaultExpirationColumnName &&
 				tableDesc.HasRowLevelTTL() &&
 				tableDesc.GetRowLevelTTL().HasDurationExpr() {
 				return pgerror.Newf(
@@ -753,7 +752,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 		case *tree.AlterTableRenameColumn:
 			tableDesc := n.tableDesc
 			columnName := t.Column
-			if columnName == colinfo.TTLDefaultExpirationColumnName &&
+			if columnName == catpb.TTLDefaultExpirationColumnName &&
 				tableDesc.HasRowLevelTTL() &&
 				tableDesc.GetRowLevelTTL().HasDurationExpr() {
 				return pgerror.Newf(
@@ -1841,7 +1840,7 @@ func handleTTLStorageParamChange(
 
 		// Update default expression on automated column if required.
 		if before.HasDurationExpr() && after.HasDurationExpr() && before.DurationExpr != after.DurationExpr {
-			col, err := catalog.MustFindColumnByName(tableDesc, colinfo.TTLDefaultExpirationColumnName)
+			col, err := catalog.MustFindColumnByName(tableDesc, catpb.TTLDefaultExpirationColumnName)
 			if err != nil {
 				return false, err
 			}
@@ -1883,11 +1882,11 @@ func handleTTLStorageParamChange(
 		// Adding a TTL requires adding the automatic column and deferring the TTL
 		// addition to after the column is successfully added.
 		addTTLMutation = true
-		if catalog.FindColumnByName(tableDesc, colinfo.TTLDefaultExpirationColumnName) != nil {
+		if catalog.FindColumnByName(tableDesc, catpb.TTLDefaultExpirationColumnName) != nil {
 			return false, pgerror.Newf(
 				pgcode.InvalidTableDefinition,
 				"cannot add TTL to table with the %s column already defined",
-				colinfo.TTLDefaultExpirationColumnName,
+				catpb.TTLDefaultExpirationColumnName,
 			)
 		}
 		col, err := rowLevelTTLAutomaticColumnDef(after)
@@ -1933,7 +1932,7 @@ func handleTTLStorageParamChange(
 		// Create the DROP COLUMN job and the associated mutation.
 		dropTTLMutation = true
 		droppedViews, err := dropColumnImpl(params, tn, tableDesc, after, &tree.AlterTableDropColumn{
-			Column: colinfo.TTLDefaultExpirationColumnName,
+			Column: catpb.TTLDefaultExpirationColumnName,
 		})
 		if err != nil {
 			return false, err

pkg/sql/catalog/catpb/BUILD.bazel

@@ -41,7 +41,6 @@ go_library(
         "job_id.go",
         "multiregion.go",
         "privilege.go",
-        "ttl.go",
         ":gen-privilegedescversion-stringer",  # keep
     ],
     embed = [":catpb_go_proto"],

pkg/sql/catalog/catpb/catalog.go

@@ -99,6 +99,14 @@ func (as *AutoStatsSettings) NoAutoStatsSettingsOverrides() bool {
 	return true
 }
 
+// TTLDefaultExpirationColumnName is the column name representing the expiration
+// column for TTL.
+const TTLDefaultExpirationColumnName = "crdb_internal_expiration"
+
+// DefaultTTLExpirationExpr is default TTL expression when
+// ttl_expiration_expression is not specified
+var DefaultTTLExpirationExpr = Expression(TTLDefaultExpirationColumnName)
+
 // HasDurationExpr is a utility method to determine if ttl_expires_after was set
 func (rowLevelTTL *RowLevelTTL) HasDurationExpr() bool {
 	return rowLevelTTL.DurationExpr != ""
@@ -108,3 +116,18 @@ func (rowLevelTTL *RowLevelTTL) HasDurationExpr() bool {
 func (rowLevelTTL *RowLevelTTL) HasExpirationExpr() bool {
 	return rowLevelTTL.ExpirationExpr != ""
 }
+
+// DeletionCronOrDefault returns the DeletionCron or the global default.
+func (m *RowLevelTTL) DeletionCronOrDefault() string {
+	if override := m.DeletionCron; override != "" {
+		return override
+	}
+	return "@hourly"
+}
+
+func (rowLevelTTL *RowLevelTTL) GetTTLExpr() Expression {
+	if rowLevelTTL.HasExpirationExpr() {
+		return rowLevelTTL.ExpirationExpr
+	}
+	return DefaultTTLExpirationExpr
+}

pkg/sql/catalog/colinfo/BUILD.bazel

@@ -13,7 +13,6 @@ go_library(
         "ordering.go",
         "result_columns.go",
         "system_columns.go",
-        "ttl.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo",
     visibility = ["//visibility:public"],

pkg/sql/catalog/lease/lease.go

@@ -483,15 +483,15 @@ func (m *Manager) AcquireFreshestFromStore(ctx context.Context, id descpb.ID) er
 	// If we are to join an in-progress acquisition, it needs to be an acquisition
 	// initiated after this point.
 	// So, we handle two cases:
-	// 1. The first DoChan() call tells us that we didn't join an in-progress
-	//     acquisition. Great, the lease that's being acquired is good.
-	// 2. The first DoChan() call tells us that we did join an in-progress acq.
-	//     We have to wait this acquisition out; it's not good for us. But any
-	//     future acquisition is good, so the next time around the loop it doesn't
-	//     matter if we initiate a request or join an in-progress one.
-	// In both cases, we need to check if the lease we want is still valid because
-	// lease acquisition is done without holding the descriptorState lock, so anything
-	// can happen in between lease acquisition and us getting control again.
+	// 1. The first acquireNodeLease(..) call tells us that we didn't join an
+	//    in-progress acquisition but rather initiated one. Great, the lease
+	//    that's being acquired is good.
+	// 2. The first acquireNodeLease(..) call tells us that we did join an
+	//    in-progress acquisition;
+	//    We have to wait this acquisition out; it's not good for us. But any
+	//    future acquisition is good, so the next time around the loop it doesn't
+	//    matter if we initiate a request or join an in-progress one (because
+	//    either way, it's an acquisition performed after this call).
 	attemptsMade := 0
 	for {
 		// Acquire a fresh lease.
@@ -501,12 +501,11 @@ func (m *Manager) AcquireFreshestFromStore(ctx context.Context, id descpb.ID) er
 		}
 
 		if didAcquire {
-			// Case 1: we didn't join an in-progress call and the lease is still
-			// valid.
+			// Case 1: we initiated a lease acquisition call.
 			break
-		} else if attemptsMade > 1 {
-			// Case 2: more than one acquisition has happened and the lease is still
-			// valid.
+		} else if attemptsMade > 0 {
+			// Case 2: we joined an in-progress lease acquisition call but that call
+			//         was initiated after we entered this function.
 			break
 		}
 		attemptsMade++

pkg/sql/catalog/tabledesc/ttl.go

@@ -16,7 +16,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -94,45 +93,47 @@ func ValidateTTLExpirationExpr(desc catalog.TableDescriptor) error {
 // ValidateTTLExpirationColumn validates that the ttl_expire_after setting, if
 // any, is in a valid state. It requires that the TTLDefaultExpirationColumn
 // exists and has DEFAULT/ON UPDATE clauses.
-func ValidateTTLExpirationColumn(desc catalog.TableDescriptor) error {
+func ValidateTTLExpirationColumn(desc catalog.TableDescriptor, allowDescPK bool) error {
 	if !desc.HasRowLevelTTL() {
 		return nil
 	}
 	if !desc.GetRowLevelTTL().HasDurationExpr() {
 		return nil
 	}
 	intervalExpr := desc.GetRowLevelTTL().DurationExpr
-	col, err := catalog.MustFindColumnByTreeName(desc, colinfo.TTLDefaultExpirationColumnName)
+	col, err := catalog.MustFindColumnByTreeName(desc, catpb.TTLDefaultExpirationColumnName)
 	if err != nil {
-		return errors.Wrapf(err, "expected column %s", colinfo.TTLDefaultExpirationColumnName)
+		return errors.Wrapf(err, "expected column %s", catpb.TTLDefaultExpirationColumnName)
 	}
 	expectedStr := `current_timestamp():::TIMESTAMPTZ + ` + string(intervalExpr)
 	if col.GetDefaultExpr() != expectedStr {
 		return pgerror.Newf(
 			pgcode.InvalidTableDefinition,
 			"expected DEFAULT expression of %s to be %s",
-			colinfo.TTLDefaultExpirationColumnName,
+			catpb.TTLDefaultExpirationColumnName,
 			expectedStr,
 		)
 	}
 	if col.GetOnUpdateExpr() != expectedStr {
 		return pgerror.Newf(
 			pgcode.InvalidTableDefinition,
 			"expected ON UPDATE expression of %s to be %s",
-			colinfo.TTLDefaultExpirationColumnName,
+			catpb.TTLDefaultExpirationColumnName,
 			expectedStr,
 		)
 	}
 
 	// For row-level TTL, only ascending PKs are permitted.
-	pk := desc.GetPrimaryIndex()
-	for i := 0; i < pk.NumKeyColumns(); i++ {
-		dir := pk.GetKeyColumnDirection(i)
-		if dir != catenumpb.IndexColumn_ASC {
-			return unimplemented.NewWithIssuef(
-				76912,
-				`non-ascending ordering on PRIMARY KEYs are not supported with row-level TTL`,
-			)
+	if !allowDescPK {
+		pk := desc.GetPrimaryIndex()
+		for i := 0; i < pk.NumKeyColumns(); i++ {
+			dir := pk.GetKeyColumnDirection(i)
+			if dir != catenumpb.IndexColumn_ASC {
+				return unimplemented.NewWithIssuef(
+					76912,
+					`non-ascending ordering on PRIMARY KEYs are not supported with row-level TTL`,
+				)
+			}
 		}
 	}
 

pkg/sql/catalog/tabledesc/validate.go

@@ -11,6 +11,7 @@
 package tabledesc
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -862,7 +863,7 @@ func (desc *wrapper) ValidateSelf(vea catalog.ValidationErrorAccumulator) {
 	// ValidateRowLevelTTL is also used before the table descriptor is fully
 	// initialized to validate the storage parameters.
 	vea.Report(ValidateTTLExpirationExpr(desc))
-	vea.Report(ValidateTTLExpirationColumn(desc))
+	vea.Report(ValidateTTLExpirationColumn(desc, vea.IsActive(clusterversion.V23_2TTLAllowDescPK)))
 
 	// Validate that there are no column with both a foreign key ON UPDATE and an
 	// ON UPDATE expression. This check is made to ensure that we know which ON