From 33fda25ca5a5200fad42374289efcbc9c53c99fa Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Tue, 18 Apr 2023 17:18:27 -0400 Subject: [PATCH 1/2] kv: don't preemptively refresh under weak isolation levels Informs #100131. This commit disables preemptive transaction refreshes under weak isolation levels. These transactions can tolerate write skew, so they can commit even if their write timestamp has been bumped. Transactions run at weak isolation levels may refresh in response to WriteTooOld errors or ReadWithinUncertaintyInterval errors returned by requests, but they do not need to refresh preemptively ahead of an EndTxn request. Release note: None --- .../kvcoord/txn_interceptor_span_refresher.go | 9 +++ .../txn_interceptor_span_refresher_test.go | 73 +++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go b/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go index c4ac1bc01b44..867d9b29fbda 100644 --- a/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go +++ b/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go @@ -432,6 +432,15 @@ func (sr *txnSpanRefresher) maybeRefreshPreemptivelyLocked( return ba, nil } + // If the transaction can tolerate write skew, no preemptive refresh is + // necessary, even if its write timestamp has been bumped. Transactions run at + // weak isolation levels may refresh in response to WriteTooOld errors or + // ReadWithinUncertaintyInterval errors returned by requests, but they do not + // need to refresh preemptively ahead of an EndTxn request. + if ba.Txn.IsoLevel.ToleratesWriteSkew() { + return ba, nil + } + // If true, tryRefreshTxnSpans will trivially succeed. refreshFree := ba.CanForwardReadTimestamp diff --git a/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go b/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go index a7fe6f47c93b..fd1ecde63d60 100644 --- a/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go +++ b/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go @@ -17,6 +17,7 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -539,6 +540,78 @@ func TestTxnSpanRefresherPreemptiveRefresh(t *testing.T) { require.False(t, tsr.refreshInvalid) } +// TestTxnSpanRefresherPreemptiveRefreshIsoLevel tests that the txnSpanRefresher +// only performed preemptive client-side refreshes of Serializable transactions. +func TestTxnSpanRefresherPreemptiveRefreshIsoLevel(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + tests := []struct { + isoLevel isolation.Level + expRefresh bool + }{ + {isolation.Serializable, true}, + {isolation.Snapshot, false}, + {isolation.ReadCommitted, false}, + } + for _, tt := range tests { + t.Run(tt.isoLevel.String(), func(t *testing.T) { + ctx := context.Background() + tsr, mockSender := makeMockTxnSpanRefresher() + + txn := makeTxnProto() + txn.IsoLevel = tt.isoLevel + + // Push the txn. + txn.WriteTimestamp = txn.WriteTimestamp.Add(1, 0) + origReadTs := txn.ReadTimestamp + pushedWriteTs := txn.WriteTimestamp + + // Send an EndTxn request. + ba := &kvpb.BatchRequest{} + ba.Header = kvpb.Header{Txn: &txn} + etArgs := kvpb.EndTxnRequest{Commit: true} + ba.Add(&etArgs) + + mockSender.MockSend(func(ba *kvpb.BatchRequest) (*kvpb.BatchResponse, *kvpb.Error) { + require.Len(t, ba.Requests, 1) + require.True(t, ba.CanForwardReadTimestamp) + require.IsType(t, &kvpb.EndTxnRequest{}, ba.Requests[0].GetInner()) + + if tt.expRefresh { + // The transaction should be refreshed. + require.NotEqual(t, origReadTs, ba.Txn.ReadTimestamp) + require.Equal(t, pushedWriteTs, ba.Txn.ReadTimestamp) + require.Equal(t, pushedWriteTs, ba.Txn.WriteTimestamp) + } else { + // The transaction should not be refreshed. + require.Equal(t, origReadTs, ba.Txn.ReadTimestamp) + require.NotEqual(t, pushedWriteTs, ba.Txn.ReadTimestamp) + require.Equal(t, pushedWriteTs, ba.Txn.WriteTimestamp) + } + + br := ba.CreateReply() + br.Txn = ba.Txn + return br, nil + }) + + br, pErr := tsr.SendLocked(ctx, ba) + require.Nil(t, pErr) + require.NotNil(t, br) + + expRefreshSuccess := int64(0) + if tt.expRefresh { + expRefreshSuccess = 1 + } + require.Equal(t, expRefreshSuccess, tsr.refreshSuccess.Count()) + require.Equal(t, int64(0), tsr.refreshFail.Count()) + require.Equal(t, int64(0), tsr.refreshAutoRetries.Count()) + require.True(t, tsr.refreshFootprint.empty()) + require.False(t, tsr.refreshInvalid) + }) + } +} + // TestTxnSpanRefresherSplitEndTxnOnAutoRetry tests that EndTxn requests are // split into their own sub-batch on auto-retries after a successful refresh. // This is done to avoid starvation. From f43bd3879d75beffcf286ff0411ade38dc071eae Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 21 Apr 2023 08:49:42 +0200 Subject: [PATCH 2/2] sql: fix the pretty-printing of CREATE EXTENSION Release note: None --- pkg/sql/create_extension.go | 6 +++--- pkg/sql/parser/sql.y | 4 ++-- pkg/sql/parser/testdata/create_misc | 8 ++++++++ pkg/sql/sem/tree/create.go | 6 ++++-- pkg/sql/testdata/telemetry/extension | 2 +- 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/pkg/sql/create_extension.go b/pkg/sql/create_extension.go index a37dff64ed7c..5931a12d34b8 100644 --- a/pkg/sql/create_extension.go +++ b/pkg/sql/create_extension.go @@ -35,7 +35,7 @@ func (n *createExtensionNode) unimplementedExtensionError(issue int) error { name := n.CreateExtension.Name return unimplemented.NewWithIssueDetailf( issue, - "CREATE EXTENSION "+name, + "CREATE EXTENSION "+string(name), "extension %q is not yet supported", name, ) @@ -48,7 +48,7 @@ func (n *createExtensionNode) startExec(params runParams) error { "fuzzystrmatch", "pgcrypto", "uuid-ossp": - telemetry.Inc(sqltelemetry.CreateExtensionCounter(n.CreateExtension.Name)) + telemetry.Inc(sqltelemetry.CreateExtensionCounter(string(n.CreateExtension.Name))) return nil case "postgis_raster", "postgis_topology", @@ -105,7 +105,7 @@ func (n *createExtensionNode) startExec(params runParams) error { "xml2": return n.unimplementedExtensionError(54516) } - return pgerror.Newf(pgcode.UndefinedParameter, "unknown extension: %q", n.CreateExtension.Name) + return pgerror.Newf(pgcode.UndefinedParameter, "unknown extension: %s", n.CreateExtension.Name) } func (n *createExtensionNode) Next(params runParams) (bool, error) { return false, nil } diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index d0264d99325f..3a678f2b5bd3 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -4490,10 +4490,10 @@ create_schedule_stmt: create_extension_stmt: CREATE EXTENSION IF NOT EXISTS name { - $$.val = &tree.CreateExtension{IfNotExists: true, Name: $6} + $$.val = &tree.CreateExtension{IfNotExists: true, Name: tree.Name($6)} } | CREATE EXTENSION name { - $$.val = &tree.CreateExtension{Name: $3} + $$.val = &tree.CreateExtension{Name: tree.Name($3)} } | CREATE EXTENSION IF NOT EXISTS name WITH error { diff --git a/pkg/sql/parser/testdata/create_misc b/pkg/sql/parser/testdata/create_misc index 05c2a13dcb12..f1b4983cfffc 100644 --- a/pkg/sql/parser/testdata/create_misc +++ b/pkg/sql/parser/testdata/create_misc @@ -9,6 +9,14 @@ CREATE EXTENSION bob -- fully parenthesized CREATE EXTENSION bob -- literals removed CREATE EXTENSION bob -- identifiers removed +parse +CREATE EXTENSION "a-b" +---- +CREATE EXTENSION "a-b" +CREATE EXTENSION "a-b" -- fully parenthesized +CREATE EXTENSION "a-b" -- literals removed +CREATE EXTENSION "a-b" -- identifiers removed + parse CREATE EXTENSION IF NOT EXISTS bob ---- diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go index d1263a73cbea..a2f5d503f5c0 100644 --- a/pkg/sql/sem/tree/create.go +++ b/pkg/sql/sem/tree/create.go @@ -2132,7 +2132,7 @@ func (o *CreateStatsOptions) CombineWith(other *CreateStatsOptions) error { // CreateExtension represents a CREATE EXTENSION statement. type CreateExtension struct { - Name string + Name Name IfNotExists bool } @@ -2147,7 +2147,9 @@ func (node *CreateExtension) Format(ctx *FmtCtx) { // do not contain sensitive information and // 2) we want to get telemetry on which extensions // users attempt to load. - ctx.WriteString(node.Name) + ctx.WithFlags(ctx.flags&^FmtAnonymize, func() { + ctx.FormatNode(&node.Name) + }) } // CreateExternalConnection represents a CREATE EXTERNAL CONNECTION statement. diff --git a/pkg/sql/testdata/telemetry/extension b/pkg/sql/testdata/telemetry/extension index 7027f9bc2067..6b8b4317cad7 100644 --- a/pkg/sql/testdata/telemetry/extension +++ b/pkg/sql/testdata/telemetry/extension @@ -28,4 +28,4 @@ unimplemented.#54516.CREATE EXTENSION xml2 feature-usage CREATE EXTENSION asdf ---- -error: pq: unknown extension: "asdf" +error: pq: unknown extension: asdf