Merge #81108 #81123 #81213

81108: *: improve inline code comments r=maryliag a=Azhng

Release note: None

81123: sql : sequence value out of bounds r=rafiss a=mosquito2333

Previously, an additional out-of-range sequence value may have been
generated.
The original criterion was whether the previous endSequence value exceeded the limit.
However, when the current endSequence is 1, the increment is 2 and the MaxValue is 2,
the previous endSequence value is 1 and the program does not report an error.
However, when the current endSequenceValue 3 exceeds the limit, an error should be reported

In incrementSequenceUsingCache, the criterion for determining whether a cache's data is eligible is modified to whether the first data that may be stored in the cache exceeds the limit.

Fixes #74127.

Release note (bug fix): Fixed a bug where sequences could return values that are out-of-bounds in some cases.

81213: ui, changefeeds: better status col in db console r=abarganier,HonoreDB a=dhartunian

Previously, when a job contained a `highwater_timestamp` column value
(present for changefeeds) the status column in DB console would
*always* show that value instead of the job status ("running", "paused",
etc.). This would cause confusion for operators because the SQL output
for job status always included both a `status` column and a separate
`highwater_timestamp` column.

This change moves the `highwater_timestamp` into a separate column and
always renders the `status` column with the "pill" component that shows
the current job status.

The highwater timestamp is also moved to the sidebar in the job details
page instead of replacing the status pill, for similar consistency.

Finally, the highwater timestamp now displays the nanosecond decimal
value by default and the human-readable formatted value in the tooltip.
This faciliates easier copy/paste behavior from the UI as the decimal is
more useful.

![Screenshot 2022-05-12 at 16-09-19 Jobs Cockroach Console](https://user-images.githubusercontent.com/986307/168162734-48a61ec4-c5a5-41e2-9533-845afe08a0b4.png)
![Screenshot 2022-05-12 at 16-09-47 Details Job Cockroach Console](https://user-images.githubusercontent.com/986307/168162742-ee3e3f4c-136e-424f-af54-cf03fde42406.png)


Resolves #80496

Release note (ui change): The job status page in the DB Console will now
show the status column for changefeed jobs and display the
`highwater_timestamp` value in a separate column. Thise more closely
matches the SQL output of `SHOW changefeed JOBS`. The highwater
timestamp now displays as the nanosecond system time value by default
with the human-readable value in the tooltip since the decimal value is
copy/pasted more often.

Co-authored-by: Azhng <[email protected]>
Co-authored-by: mosquito2333 <[email protected]>
Co-authored-by: David Hartunian <[email protected]>

pkg/sql/logictest/testdata/logic_test/sequences

@@ -2253,3 +2253,61 @@ SELECT nextval('restart_txn_seq')
 
 statement ok
 END
+
+# Regression test for #74127. Assert that out-of-range sequence values are not returned.
+
+# The value value(12) is out-of-bounds
+statement ok
+CREATE SEQUENCE customer_seq_check_cache_and_bounds_1 INCREMENT  BY 3  MINVALUE 6 MAXVALUE 10
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_1')
+----
+6
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_1')
+----
+9
+
+statement error pgcode 2200H pq: nextval\(\): reached maximum value of sequence "customer_seq_check_cache_and_bounds_1" \(10\)
+SELECT nextval('customer_seq_check_cache_and_bounds_1')
+
+# Set the cache to 5
+statement ok
+CREATE SEQUENCE customer_seq_check_cache_and_bounds_2 MINVALUE -2 MAXVALUE 2 START WITH 1 CACHE 5 INCREMENT BY -2
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_2')
+----
+1
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_2')
+----
+-1
+
+statement error pgcode 2200H pq: nextval\(\): reached minimum value of sequence "customer_seq_check_cache_and_bounds_2" \(-2\)
+SELECT nextval('customer_seq_check_cache_and_bounds_2')
+
+# The value value(12) is equal with MAXVALUE(12)
+statement ok
+CREATE SEQUENCE customer_seq_check_cache_and_bounds_3 INCREMENT  BY 3  MINVALUE 6 MAXVALUE 12
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_3')
+----
+6
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_3')
+----
+9
+
+query I
+SELECT nextval('customer_seq_check_cache_and_bounds_3')
+----
+12
+
+statement error pgcode 2200H pq: nextval\(\): reached maximum value of sequence "customer_seq_check_cache_and_bounds_3" \(12\)
+SELECT nextval('customer_seq_check_cache_and_bounds_3')

pkg/sql/sequence.go

@@ -186,8 +186,8 @@ func (p *planner) incrementSequenceUsingCache(
 		// This sequence has exceeded its bounds after performing this increment.
 		if endValue > seqOpts.MaxValue || endValue < seqOpts.MinValue {
 			// If the sequence exceeded its bounds prior to the increment, then return an error.
-			if (seqOpts.Increment > 0 && endValue-seqOpts.Increment*cacheSize >= seqOpts.MaxValue) ||
-				(seqOpts.Increment < 0 && endValue-seqOpts.Increment*cacheSize <= seqOpts.MinValue) {
+			if (seqOpts.Increment > 0 && endValue-seqOpts.Increment*(cacheSize-1) > seqOpts.MaxValue) ||
+				(seqOpts.Increment < 0 && endValue-seqOpts.Increment*(cacheSize-1) < seqOpts.MinValue) {
 				return 0, 0, 0, boundsExceededError(descriptor)
 			}
 			// Otherwise, values between the limit and the value prior to incrementing can be cached.

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go

@@ -268,6 +268,17 @@ func TestSQLStatsCompactor(t *testing.T) {
 	}
 }
 
+// TestSQLStatsForegroundInterference ensures that the background SQL Stats
+// cleanup job does not delete any rows in the current aggregation window. Doing
+// so would cause contentions, which can potentially lead to long runtime of the
+// SQL Stats cleanup job. We test this behavior by generating some rows in the
+// stats system table that are in the current aggregation window and previous
+// aggregation window. Before running the SQL Stats compaction, we lower the
+// row limit in the stats table so that all thw rows will be deleted by the
+// StatsCompactor, if all the generated rows live outside the current
+// aggregation window. This test asserts that, since some of generated rows live
+// in the current aggregation interval, those rows will not be deleted by the
+// StatsCompactor.
 func TestSQLStatsForegroundInterference(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/sql/tests/server_params.go

@@ -45,6 +45,22 @@ func CreateTestTenantParams(tenantID roachpb.TenantID) base.TestTenantArgs {
 }
 
 // CreateTestingKnobs creates a testing knob in the unit tests.
+//
+// Note: SQL Stats’s read path uses follower read
+//       (AS OF SYSTEM TIME follower_read_timestamp()) to ensure that contention
+//       between reads and writes (SQL Stats flush / SQL Stats cleanup) is
+//       minimized.
+//       However, in a new cluster in unit tests, system tables are created
+//       using the migration framework. The migration framework goes through a
+//       list of registered migrations and creates the stats system tables. By
+//       using follower read, we shift the transaction read timestamp far enough
+//       to the past. This means it is possible in the unit tests, the read
+//       timestamp would be chosen to be before the creation of the stats table.
+//       This can cause 'descriptor not found' error when accessing the stats
+//       system table.
+//       Additionally, we don't want to completely remove the AOST clause in the
+//       unit test. Therefore, `AS OF SYSTEM TIME '-1us'` is a compromise
+//       used to get around the 'descriptor not found' error.
 func CreateTestingKnobs() base.TestingKnobs {
 	return base.TestingKnobs{
 		SQLStatsKnobs: &sqlstats.TestingKnobs{

pkg/ui/workspaces/db-console/src/views/jobs/highwaterTimestamp.tsx

@@ -16,14 +16,17 @@ import { google } from "src/js/protos";
 import ITimestamp = google.protobuf.ITimestamp;
 
 interface HighwaterProps {
-  highwater: ITimestamp;
-  tooltip: string;
+  timestamp: ITimestamp;
+  decimalString: string;
 }
 
 export class HighwaterTimestamp extends React.PureComponent<HighwaterProps> {
   render() {
+    if (!this.props.timestamp) {
+      return null;
+    }
     let highwaterMoment = moment(
-      this.props.highwater.seconds.toNumber() * 1000,
+      this.props.timestamp.seconds.toNumber() * 1000,
     );
     // It's possible due to client clock skew that this timestamp could be in
     // the future. To avoid confusion, set a maximum bound of now.
@@ -33,8 +36,8 @@ export class HighwaterTimestamp extends React.PureComponent<HighwaterProps> {
     }
 
     return (
-      <ToolTipWrapper text={`System Time: ${this.props.tooltip}`}>
-        High-water Timestamp: {highwaterMoment.format(DATE_FORMAT)}
+      <ToolTipWrapper text={highwaterMoment.format(DATE_FORMAT)}>
+        {this.props.decimalString}
       </ToolTipWrapper>
     );
   }

pkg/ui/workspaces/db-console/src/views/jobs/jobDetails.tsx

@@ -31,6 +31,7 @@ import { DATE_FORMAT } from "src/util/format";
 import { JobStatusCell } from "./jobStatusCell";
 import "src/views/shared/components/summaryCard/styles.styl";
 import * as protos from "src/js/protos";
+import { HighwaterTimestamp } from "src/views/jobs/highwaterTimestamp";
 
 interface JobsTableProps extends RouteComponentProps {
   refreshJob: typeof refreshJob;
@@ -84,6 +85,21 @@ class JobDetails extends React.Component<JobsTableProps, {}> {
                   <p className="summary--card__counting--label">Users</p>
                 </div>
               </Col>
+              {job.highwater_timestamp ? (
+                <Col span={24}>
+                  <div className="summary--card__counting">
+                    <h3 className="summary--card__counting--value">
+                      <HighwaterTimestamp
+                        timestamp={job.highwater_timestamp}
+                        decimalString={job.highwater_decimal}
+                      />
+                    </h3>
+                    <p className="summary--card__counting--label">
+                      High-water Timestamp
+                    </p>
+                  </div>
+                </Col>
+              ) : null}
             </Row>
           </SummaryCard>
         </Col>

pkg/ui/workspaces/db-console/src/views/jobs/jobStatusCell.tsx

@@ -10,7 +10,6 @@
 
 import React from "react";
 import { cockroach } from "src/js/protos";
-import { HighwaterTimestamp } from "src/views/jobs/highwaterTimestamp";
 import { JobStatus } from "./jobStatus";
 import { isRetrying } from "src/views/jobs/jobStatusOptions";
 import { util } from "@cockroachlabs/cluster-ui";
@@ -29,15 +28,6 @@ export const JobStatusCell: React.FC<JobStatusCellProps> = ({
   lineWidth,
   compact = false,
 }) => {
-  if (job.highwater_timestamp) {
-    return (
-      <HighwaterTimestamp
-        highwater={job.highwater_timestamp}
-        tooltip={job.highwater_decimal}
-      />
-    );
-  }
-
   const jobStatus = (
     <JobStatus job={job} lineWidth={lineWidth} compact={compact} />
   );

pkg/ui/workspaces/db-console/src/views/jobs/jobTable.tsx

@@ -37,6 +37,7 @@ import { Anchor } from "src/components";
 import emptyTableResultsIcon from "assets/emptyState/empty-table-results.svg";
 import magnifyingGlassIcon from "assets/emptyState/magnifying-glass.svg";
 import { Tooltip } from "@cockroachlabs/ui-components";
+import { HighwaterTimestamp } from "src/views/jobs/highwaterTimestamp";
 
 class JobsSortedTable extends SortedTable<Job> {}
 
@@ -174,6 +175,26 @@ const jobsTableColumns: ColumnDescriptor<Job>[] = [
       util.TimestampToMoment(job?.last_run).format(DATE_FORMAT_24_UTC),
     sort: job => util.TimestampToMoment(job?.last_run).valueOf(),
   },
+  {
+    name: "High-water Timestamp",
+    title: (
+      <Tooltip
+        placement="bottom"
+        style="tableTitle"
+        content={<p>Date and time the job was last executed.</p>}
+      >
+        {"High-water Timestamp"}
+      </Tooltip>
+    ),
+    cell: job =>
+      job.highwater_timestamp ? (
+        <HighwaterTimestamp
+          timestamp={job.highwater_timestamp}
+          decimalString={job.highwater_decimal}
+        />
+      ) : null,
+    sort: job => util.TimestampToMoment(job?.highwater_timestamp).valueOf(),
+  },
   {
     name: "executionCount",
     title: (