From df380db13a5ad0dca05e6f7461dbf905f48acb21 Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Tue, 5 Jul 2022 10:38:51 -0400 Subject: [PATCH 1/4] sql/schemachanger: check nil table name for comment on column throw pg error instead of panic on nil pointer. Release note: None. --- .../schemachanger/scbuild/internal/scbuildstmt/comment_on.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/comment_on.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/comment_on.go index e7989ea29725..f337c3c28eab 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/comment_on.go +++ b/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) From 9272602d332b667b1a5fa373b339dd9f31414c6e Mon Sep 17 00:00:00 2001 From: Renato Costa Date: Wed, 6 Jul 2022 14:32:05 -0400 Subject: [PATCH 2/4] roachprod: fix target path when getting cluster artifacts. 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. --- pkg/roachprod/install/cluster_synced.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/roachprod/install/cluster_synced.go b/pkg/roachprod/install/cluster_synced.go index 3f09950faed7..9535bfe87ddf 100644 --- a/pkg/roachprod/install/cluster_synced.go +++ b/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) } From 056d2126a8c935c1272d86e25e893142e0ce9000 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Wed, 6 Jul 2022 19:08:37 +0000 Subject: [PATCH 3/4] storage: clarify and test overlapping SST range keys in `Export` 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 --- .../batcheval/cmd_add_sstable_test.go | 6 +++ pkg/roachpb/api.proto | 17 ++++---- pkg/storage/mvcc.go | 13 +++--- .../testdata/mvcc_histories/sst_iter_multi | 43 +++++++++++++++++++ 4 files changed, 65 insertions(+), 14 deletions(-) diff --git a/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go b/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go index d713911a2c5e..05d9189b947b 100644 --- a/pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go +++ b/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")}, diff --git a/pkg/roachpb/api.proto b/pkg/roachpb/api.proto index 09b4d9e03196..1083edb7f0eb 100644 --- a/pkg/roachpb/api.proto +++ b/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. diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index a2b8b5eed5d2..5e3af3068456 100644 --- a/pkg/storage/mvcc.go +++ b/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 diff --git a/pkg/storage/testdata/mvcc_histories/sst_iter_multi b/pkg/storage/testdata/mvcc_histories/sst_iter_multi index 2465abe7dbb3..ef1d4a19ecb0 100644 --- a/pkg/storage/testdata/mvcc_histories/sst_iter_multi +++ b/pkg/storage/testdata/mvcc_histories/sst_iter_multi @@ -267,3 +267,46 @@ iter_seek_ge: "b"/3.000000000,0=/ {b-d}/[4.000000000,0=/] iter_seek_ge: {b-d}/[4.000000000,0=/] iter_seek_ge: "b"/1.000000000,0=/BYTES/b1 {b-d}/[4.000000000,0=/] iter_seek_ge: {b-d}/[4.000000000,0=/] + +# 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: + +>> sst-0: +set: "c"/3.000000000,0 -> /BYTES/c3 +rangekeyset: {a-c\x00}/2.000000000,0 -> / +>> sst-1: +set: "c"/1.000000000,0 -> /BYTES/c3 +rangekeyset: {c-f}/2.000000000,0 -> / + +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=/] +iter_seek_ge: {a-f}/[2.000000000,0=/] +iter_seek_ge: "c"/1.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/] +iter_seek_ge: {a-f}/[2.000000000,0=/] +iter_scan: {a-f}/[2.000000000,0=/] +iter_scan: "c"/3.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/] +iter_scan: "c"/1.000000000,0=/BYTES/c3 {a-f}/[2.000000000,0=/] +iter_scan: . From 52737fba7c5aec355940e5ae948e8acaea31e5f3 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 6 Jul 2022 14:43:11 -0700 Subject: [PATCH 4/4] coexec: normalize the timestamp subtraction like postgres 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. 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. --- pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go | 8 ++++---- .../colexecprojconst/proj_const_left_ops.eg.go | 8 ++++---- .../colexecprojconst/proj_const_right_ops.eg.go | 8 ++++---- pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go | 2 +- pkg/sql/logictest/testdata/logic_test/timestamp | 11 +++++++++++ 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go b/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go index ade03095df8a..6b9f251175b2 100644 --- a/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go +++ b/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go @@ -9020,7 +9020,7 @@ func (p projMinusTimestampTimestampOp) Next() coldata.Batch { arg2 := col2.Get(i) nanos := arg1.Sub(arg2).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } @@ -9038,7 +9038,7 @@ func (p projMinusTimestampTimestampOp) Next() coldata.Batch { arg2 := col2.Get(i) nanos := arg1.Sub(arg2).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } @@ -9057,7 +9057,7 @@ func (p projMinusTimestampTimestampOp) Next() coldata.Batch { arg2 := col2.Get(i) nanos := arg1.Sub(arg2).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } else { @@ -9071,7 +9071,7 @@ func (p projMinusTimestampTimestampOp) Next() coldata.Batch { arg2 := col2.Get(i) nanos := arg1.Sub(arg2).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } diff --git a/pkg/sql/colexec/colexecprojconst/proj_const_left_ops.eg.go b/pkg/sql/colexec/colexecprojconst/proj_const_left_ops.eg.go index 43a86a594233..c11ae18c5927 100644 --- a/pkg/sql/colexec/colexecprojconst/proj_const_left_ops.eg.go +++ b/pkg/sql/colexec/colexecprojconst/proj_const_left_ops.eg.go @@ -8406,7 +8406,7 @@ func (p projMinusTimestampConstTimestampOp) Next() coldata.Batch { arg := col.Get(i) nanos := p.constArg.Sub(arg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } @@ -8420,7 +8420,7 @@ func (p projMinusTimestampConstTimestampOp) Next() coldata.Batch { arg := col.Get(i) nanos := p.constArg.Sub(arg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } @@ -8438,7 +8438,7 @@ func (p projMinusTimestampConstTimestampOp) Next() coldata.Batch { arg := col.Get(i) nanos := p.constArg.Sub(arg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } else { @@ -8449,7 +8449,7 @@ func (p projMinusTimestampConstTimestampOp) Next() coldata.Batch { arg := col.Get(i) nanos := p.constArg.Sub(arg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } diff --git a/pkg/sql/colexec/colexecprojconst/proj_const_right_ops.eg.go b/pkg/sql/colexec/colexecprojconst/proj_const_right_ops.eg.go index b677ed268c4e..6fea899f61da 100644 --- a/pkg/sql/colexec/colexecprojconst/proj_const_right_ops.eg.go +++ b/pkg/sql/colexec/colexecprojconst/proj_const_right_ops.eg.go @@ -8403,7 +8403,7 @@ func (p projMinusTimestampTimestampConstOp) Next() coldata.Batch { arg := col.Get(i) nanos := arg.Sub(p.constArg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } @@ -8417,7 +8417,7 @@ func (p projMinusTimestampTimestampConstOp) Next() coldata.Batch { arg := col.Get(i) nanos := arg.Sub(p.constArg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } @@ -8435,7 +8435,7 @@ func (p projMinusTimestampTimestampConstOp) Next() coldata.Batch { arg := col.Get(i) nanos := arg.Sub(p.constArg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } else { @@ -8446,7 +8446,7 @@ func (p projMinusTimestampTimestampConstOp) Next() coldata.Batch { arg := col.Get(i) nanos := arg.Sub(p.constArg).Nanoseconds() - projCol[i] = duration.MakeDuration(nanos, 0, 0) + projCol[i] = duration.MakeDurationJustifyHours(nanos, 0, 0) } } diff --git a/pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go b/pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go index 48a311adddab..d8420bab136e 100644 --- a/pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go +++ b/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: diff --git a/pkg/sql/logictest/testdata/logic_test/timestamp b/pkg/sql/logictest/testdata/logic_test/timestamp index b428f1dd1a4d..9ceaeca7cb78 100644 --- a/pkg/sql/logictest/testdata/logic_test/timestamp +++ b/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