Merge #83812 #83923 #83927 #83944

83812: sql/schemachanger: check nil table name for comment on column r=chengxiong-ruan a=chengxiong-ruan

addressing this [build error](#75604 (comment))
throw pg error instead of panic on nil pointer.

Release note: None.

83923: roachprod: fix target path when getting cluster artifacts. r=srosenberg a=renatolabs

A bug was introduced in the computation of the local path where
cluster artifacts are downloaded: instead of prefixing the path with
the node ID, the list of nodes would be used instead. This results
in (other than weird looking paths, such as `[1 2 3 4].logs`) only the
logs for the last node being downloaded.

This commit fixes the issue by using the node ID correctly.

Release note: None.

83927: storage: clarify and test overlapping SST range keys in `Export` r=nicktrav a=erikgrinaker

This patch clarifies that an `Export` with overlapping MVCC range
tombstones across two SSTs due to `SplitMidKey` will be handled
correctly both by multiplexed iteration with `NewPebbleSSTIterator()`,
and by `AddSSTable`.

Release note: None

83944: coexec: normalize the timestamp subtraction like postgres r=yuzefovich a=yuzefovich

This commit fixes an oversight of #56667 (which fixed the normalization
of the timestamp subtraction) where we forgot to update the vectorized
code path as well. The regression tests added in that PR happened to be
complex enough so that the vectorized engine would fall back to wrapping
a row-by-row processor, so they didn't catch this.

Fixes: #83094.

Release note (bug fix): CockroachDB previously would not normalize
`timestamp/timestamptz - timestamp/timestamptz` like Postgres does in
some cases (depending on the query), and this is now fixed.

Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go

@@ -118,6 +118,12 @@ func TestEvalAddSSTable(t *testing.T) {
 			expect:         kvs{rangeKV("a", "d", 1, "")},
 			expectStatsEst: true,
 		},
+		"blind extends overlapping range tombstone": {
+			data:           kvs{rangeKV("c", "e", 1, "")},
+			sst:            kvs{rangeKV("d", "f", 1, "")},
+			expect:         kvs{rangeKV("c", "f", 1, "")},
+			expectStatsEst: true,
+		},
 		"blind rejects SST inline values under race only": { // unfortunately, for performance
 			sst:            kvs{pointKV("a", 0, "inline")},
 			expect:         kvs{pointKV("a", 0, "inline")},

pkg/roachpb/api.proto

@@ -1526,14 +1526,15 @@ message ExportRequest {
   // blowing up on memory usage. This option is only allowed together with
   // return_sst since caller should reconstruct full tables.
   //
-  // NB: If the result contains MVCC range tombstones, this can cause MVCC range
-  // tombstones in two subsequent SSTs to overlap. For example, given the range
-  // tombstone [a-f)@5, if we return a resume key at c@3 then the response will
-  // contain a truncated MVCC range tombstone [a-c\0)@5 which covers the point
-  // keys at c, but resuming from c@3 will contain the MVCC range tombstone
-  // [c-f)@5 which overlaps with the MVCC range tombstone in the previous
-  // response for the interval [c-c\0)@5. AddSSTable will allow this overlap
-  // during ingestion.
+	// NB: If the result contains MVCC range tombstones, this can cause MVCC range
+	// tombstones in two subsequent SSTs to overlap. For example, given the range
+	// tombstone [a-f)@5, if we stop between c@4 and c@2 and return a resume key c@2,
+	// then the response will contain a truncated MVCC range tombstone [a-c\0)@5
+	// which covers the point key at c, but resuming from c@2 will contain the
+	// MVCC range tombstone [c-f)@5 which overlaps with the MVCC range tombstone
+	// in the previous response in the interval [c-c\0)@5. This overlap will not
+	// cause problems with multiplexed iteration using NewPebbleSSTIterator(),
+	// nor when ingesting the SSTs via `AddSSTable`.
   bool split_mid_key = 13;
 
   // Return the exported SST data in the response.

pkg/roachprod/install/cluster_synced.go

@@ -1846,7 +1846,7 @@ func (c *SyncedCluster) Get(l *logger.Logger, nodes Nodes, src, dest string) err
 			src := src
 			dest := dest
 			if len(nodes) > 1 {
-				base := fmt.Sprintf("%d.%s", nodes, filepath.Base(dest))
+				base := fmt.Sprintf("%d.%s", nodes[i], filepath.Base(dest))
 				dest = filepath.Join(filepath.Dir(dest), base)
 			}
 

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go

@@ -599,7 +599,7 @@ func (c timestampCustomizer) getBinOpAssignFunc() assignFunc {
 		case treebin.Minus:
 			return fmt.Sprintf(`
 		  nanos := %[2]s.Sub(%[3]s).Nanoseconds()
-		  %[1]s = duration.MakeDuration(nanos, 0, 0)
+		  %[1]s = duration.MakeDurationJustifyHours(nanos, 0, 0)
 		  `,
 				targetElem, leftElem, rightElem)
 		default:

pkg/sql/logictest/testdata/logic_test/timestamp

@@ -539,3 +539,14 @@ query T
 SELECT to_timestamp(NULL)
 ----
 NULL
+
+# Regression test for #83094 (vectorized engine incorrectly formatting an
+# interval).
+statement ok
+CREATE TABLE t (t1 timestamptz, t2 timestamptz);
+INSERT INTO t VALUES ('2022-01-01 00:00:00.000000+00:00', '2022-01-02 00:00:00.000000+00:00');
+
+query T
+SELECT (t2 - t1) FROM t
+----
+1 day

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/comment_on.go

@@ -11,6 +11,8 @@
 package scbuildstmt
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl"
@@ -102,6 +104,9 @@ func CommentOnTable(b BuildCtx, n *tree.CommentOnTable) {
 
 // CommentOnColumn implements COMMENT ON COLUMN xxx IS xxx statement.
 func CommentOnColumn(b BuildCtx, n *tree.CommentOnColumn) {
+	if n.ColumnItem.TableName == nil {
+		panic(pgerror.New(pgcode.Syntax, "column name must be qualified"))
+	}
 	tableElements := b.ResolveTable(n.ColumnItem.TableName, commentResolveParams)
 	_, _, table := scpb.FindTable(tableElements)
 	columnElements := b.ResolveColumn(table.TableID, n.ColumnItem.ColumnName, commentResolveParams)

pkg/storage/mvcc.go

@@ -5060,12 +5060,13 @@ type MVCCExportOptions struct {
 	//
 	// NB: If the result contains MVCC range tombstones, this can cause MVCC range
 	// tombstones in two subsequent SSTs to overlap. For example, given the range
-	// tombstone [a-f)@5, if we return a resume key at c@3 then the response will
-	// contain a truncated MVCC range tombstone [a-c\0)@5 which covers the point
-	// keys at c, but resuming from c@3 will contain the MVCC range tombstone
-	// [c-f)@5 which overlaps with the MVCC range tombstone in the previous
-	// response for the interval [c-c\0)@5. AddSSTable will allow this overlap
-	// during ingestion.
+	// tombstone [a-f)@5, if we stop between c@4 and c@2 and return a resume key c@2,
+	// then the response will contain a truncated MVCC range tombstone [a-c\0)@5
+	// which covers the point key at c, but resuming from c@2 will contain the
+	// MVCC range tombstone [c-f)@5 which overlaps with the MVCC range tombstone
+	// in the previous response in the interval [c-c\0)@5. This overlap will not
+	// cause problems with multiplexed iteration using NewPebbleSSTIterator(),
+	// nor when ingesting the SSTs via `AddSSTable`.
 	StopMidKey bool
 	// ResourceLimiter limits how long iterator could run until it exhausts allocated
 	// resources. Export queries limiter in its iteration loop to break out once

pkg/storage/testdata/mvcc_histories/sst_iter_multi

@@ -267,3 +267,46 @@ iter_seek_ge: "b"/3.000000000,0=/<empty> {b-d}/[4.000000000,0=/<empty>]
 iter_seek_ge: {b-d}/[4.000000000,0=/<empty>]
 iter_seek_ge: "b"/1.000000000,0=/BYTES/b1 {b-d}/[4.000000000,0=/<empty>]
 iter_seek_ge: {b-d}/[4.000000000,0=/<empty>]
+
+# Clear the span. Write two SSTs:
+#
+# 1. c@3 point key, [a-c\0)@2 range key.
+# 2. c@1 point key, [c-f)@2 range keys.
+#
+# This simulates what would happen in an Export with SplitMidKey that
+# emits a resume span at c@1, where we have to ensure that the "c"
+# point is covered by the range key in both SSTs, so they will
+# overlap at [c-c\0)@2.
+run ok
+sst_reset
+sst_put k=c ts=3 v=c3
+sst_put_rangekey k=a end=+c ts=2
+sst_finish
+sst_put k=c ts=1 v=c3
+sst_put_rangekey k=c end=f ts=2
+----
+>> at end:
+<no data>
+>> sst-0:
+set: "c"/3.000000000,0 -> /BYTES/c3
+rangekeyset: {a-c\x00}/2.000000000,0 -> /<empty>
+>> sst-1:
+set: "c"/1.000000000,0 -> /BYTES/c3
+rangekeyset: {c-f}/2.000000000,0 -> /<empty>
+
+run ok
+sst_iter_new
+iter_seek_ge k=c ts=3
+iter_seek_ge k=c ts=2
+iter_seek_ge k=c ts=1
+iter_seek_ge k=a
+iter_scan
+----
+iter_seek_ge: "c"/3.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/<empty>]
+iter_seek_ge: {a-f}/[2.000000000,0=/<empty>]
+iter_seek_ge: "c"/1.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/<empty>]
+iter_seek_ge: {a-f}/[2.000000000,0=/<empty>]
+iter_scan: {a-f}/[2.000000000,0=/<empty>]
+iter_scan: "c"/3.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/<empty>]
+iter_scan: "c"/1.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/<empty>]
+iter_scan: .