From eb177d3ec7d6cccc3e7c29431832443bf3956f83 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Fri, 19 Feb 2021 18:26:54 -0500 Subject: [PATCH 1/7] docs: Add ui ARCHITECTURE.md doc Add an ARCHITECTURE doc to /pkg/ui to help developers onboard to the project. The doc contains pointers on general React and Redux knowledge and links on where best to acquire it as well as a dataflow diagram and some FAQs on how data flows through the application, which is likely the part that's hardest to currently understand. Release note: None --- pkg/ui/ARCHITECTURE.md | 145 +++++++++++++++++++++++++++++++++++++++++ pkg/ui/README.md | 8 +++ 2 files changed, 153 insertions(+) create mode 100644 pkg/ui/ARCHITECTURE.md diff --git a/pkg/ui/ARCHITECTURE.md b/pkg/ui/ARCHITECTURE.md new file mode 100644 index 000000000000..c8f2e6ad2028 --- /dev/null +++ b/pkg/ui/ARCHITECTURE.md @@ -0,0 +1,145 @@ +# DB Console Architecture + +## Language, Tools, and Frameworks + +This repository uses the Typescript language, and the React, and Redux libraries to +build a large complex UI app. Familiarity with these 3 technologies is critical +to understanding what's going on and making changes with intention. + +All three technologies have wonderful documentation which you're encouraged to +start with when learning. + +### Why it's hard +> A big obstacle to understanding how React and Redux work is the fact +> that both libraries **hide the call graph** from your application. +> This can make it hard to mentally trace code because the calls connecting +> portions of your app are simply not in your codebase. You are required +> to understand _some_ React and Redux internals to make the application's +> architecture legible. + +### React + +**Why?** React is a library for building interactive UIs. It lets us use a +"Component" pattern to define composable views based on what data inputs we have +and then the library takes care of updating those views when the data changes. + +The React docs are very good at quickly walking you through what you need to +know. You can either follow a +[Practical Tutorial](https://reactjs.org/tutorial/tutorial.html) or a tour of +[Main Concepts](https://reactjs.org/docs/hello-world.html). Both are great, not +too long, and will teach you what you need to be productive. + +**Note: It will be hard for you to make progress if you don't understand JSX syntax, and +what "props" and "state" are with respect to React Components.** + +In addition, the [Thinking in React](https://reactjs.org/docs/thinking-in-react.html) +page contains some great paradigm-shifting ideas and also links to the [props vs state](https://reactjs.org/docs/faq-state.html#what-is-the-difference-between-state-and-props) FAQ +question that is usually the first source of confusion for new developers. + +The [React lifecycle diagram](https://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/) +is a great tool for understanding how simple React's mental model can be. The key +to understanding how the Component model works is knowing that anytime the props +or state change within your Component, the React runtime will automatically call +`render()` again on your component and all the components underneath it in the +hierarchy to determine if anything new should be rendered to the DOM. + +### Redux + +**Why?** Redux is a library for managing complex application state in your UI. We +use it to retrieve and store all the information DB Console needs from CRDB in +one big tree of data and then slice portions of it out to feed into React +Components for rendering. In addition, any user interaction with the app that +requires interaction with CRDB, will almost certainly pass through the Redux +framework. + +The Redux docs are very detailed but approachable and use simple examples. It is +recommended that you work through the [Overview](https://redux.js.org/tutorials/fundamentals/part-1-overview) to get a high-level understanding. +In particular the [data flow diagram](https://redux.js.org/tutorials/fundamentals/part-1-overview#data-flow) can help with the basics. + +**Note: it will be hard for you to make progress if you don't understand what the +store, reducers, and actions are at a high level.** + +For the most part, change you make to DB Console will involve reading in new data +from existing endpoints in order to render something different. Your focus should +be on the Redux **selectors** which are functions of the global state. + +More specifically, we use the [`reselect` library](https://redux.js.org/recipes/computing-derived-data#creating-a-memoized-selector) that provides helpers for +automatically creating memoized selector functions of our global state. For +example, one place where we use these is in the Statements Page to compute the +data we need to render inside our table. The data provided by CRDB isn't in a + format that can be directly put into the table component so we need to do some +pre-processing on it (whether the UI should really be responsible for this is +outside the scope of this doc but that's certainly a good question to ask!). + +#### Concrete example: Statements Page + +Here's a diagram that can help explain how the data on the Statements Page table +gets there: + +![@startuml +title "How does the Statements Page get its data when the page loads?" +React -> "StatementsPage.tsx": render() +React -> "StatementsPage.tsx": componentDidMount() +"StatementsPage.tsx" -> Redux: Dispatch refresh() +Redux -> "cachedDataReducer.ts/refresh": refresh() +"cachedDataReducer.ts/refresh" -> Redux: Dispatch action: cockroachui/CachedDataReducer/statements/REQUEST +Redux -> "cachedDataReducer.ts/reducer": Run reducer with REQUEST action +"cachedDataReducer.ts/reducer" -> Redux: Returns new state for cachedData/statements with `inFlight: true` +"cachedDataReducer.ts/refresh" -> CRDB: HTTP call to API endpoint +CRDB -> "cachedDataReducer.ts/refresh": Invoke callback on response +note right +async +end note +"cachedDataReducer.ts/refresh" -> Redux: Dispatch action: cockroachui/CachedDataReducer/statements/RECEIVE\nwith payload containing statements data +Redux -> "cachedDataReducer.ts/reducer": Run reducer with RECEIVE action +"cachedDataReducer.ts/reducer" -> Redux: Returns new state for cachedData/statements subhierarchy with\nnew payload added to state +Redux -> Redux: Merges cachedData/statements with entire application state +Redux -> "StatementsPage.tsx/selectStatements": Triggers recomputation of selectors that\nread Statements data +note right +When selectors are defined +using `createSelector` Redux +is able to track which portion +of the state can trigger their +recomputation when it changes +end note +"StatementsPage.tsx/selectStatements" -> "StatementsPage.tsx/selectStatements": New version of processed\nStatements data is generated +"StatementsPage.tsx/selectStatements" -> React: Output of selectStatements\nis a prop of the StatementsPage\ncomponent so once it's changed\nReact triggers a re-render +note right +The `selectStatements` selector +is bound to the component's props +using the `connect` function in +StatementsPage.tsx which links +the output of selectors to props +the Component expects as input +end note +React -> "StatementsPage.tsx": render() +@enduml](http://www.plantuml.com/plantuml/png/lLL1SzfC3BtxLsYuV5yFANSERLgWanpIfa3R2mUMNO5tSBIUNJaa_xxIujW9pIHqEkq9izBJUtgIr-U9JUJcfYhOSuKmk0XxS04JS8amPyDuWyG9hiqMOOiCNluummRs9LBEgZLK1UFI-q4nGsCPpjx1e0ShzYsdky488fB3-F-Rr_9ikAa3oU74kwlG40lakKojC4FNt8rWubDjs9R2iOcOIa7aI2QnnfRe9g9Rpon6GG_RHA7h8IzcFaidVVX0AjdkOX1quuVZuoB3r6aVpgPVlqtdYzVLvKTHDsi8sd-mzrn2Mw6bBbx6zvhbXvj82GZta0N19aJeqOzK7eXMdZvLVblo23Wsk2fEy6SyctmSmSLYSIsLgmeum8VhIq1oTV34XSPFcSabtOOTvXfhOtSGr8HK1qfOK624gC8A09FkoHPI7_JutqnmFBtyFbrIDgaszxhz0YSsdZnjeS_DxyeVZJfJ_TLHfsPTUemcsl8-iov9O5rVnZbqEiOCwNjfcQumRZ6zj4Nov2E2gUlAMwDz79TwvjKU9gpGSXyGTnOoyYt6117rWcZuK2niu90S9CIbuIL55E7peoaysPeV9T8Zc1613ZUUq4cmIJh5bPKoZFCsQNNeMC9UyjSLgYSSTJVtfRUo227c8O4guX9RuwqXu8DoFVMnWAC6ybNg6MnfIBpiT_aaNtx3mCyorbini7MjZi5YIkYMTEILjhX5mYYdxdGP-LOVmPU6fJTbECvQadgdnFM3IKzhBwcx-Y4526GHFF-NMcz4QUPuC5IBHJmxV5QU3dWXjLV7_Ajkv8SnhaD3kjjPISSiTAemTPl0Mii68e6kODEGpNFpEkjVlMdNeVAqqn8A3hqZ_QQ6ZaLJnbtVU5TBIWAJX45W_JwS-dKzbmVvgFy4) + +#### Q: I have new data coming in through an existing endpoint, how do I access it? +You may need to modify a **selector** function to pass the new piece of data down +to the component's props for rendering. Otherwise, it may just transparently be +available in your component's props if it's simply adding a new field and your +selectors aren't radically changing the data shape after it comes back from the +API. + +#### Q: I have a new endpoint I want to hook up to +You will need to define a new `cachedDataReducer` instance or something similar +if you want to add a big chunk of data to the app state. See `apiReducers.ts` +for examples of how these are defined. + +#### Q: What does the global application state look like? +You can see this using the Redux DevTools plugin. Open DB Console, then launch +DevTools, click on the Redux tab, and then click "State" in the section selector, +and then the "Tree" tab right below. This will show you an interactive tree with +the application state. + +#### Q: What is the `CachedDataReducer`? +This is a small internally created library that manages API calls to CRDB and +tracks their execution with Redux. It makes the access of endpoints somewhat +uniform in nature. In addition the library can automatically refresh the data +for you and report errors if it can't be retrieved etc. + +_Note: Many Redux "best practices" are quite new so documentation you read online +likely won't reflect the patterns you see in this codebase today. It's up to you +to assess how valuable it might be to refactor or write new Redux code to build +your feature_ diff --git a/pkg/ui/README.md b/pkg/ui/README.md index accc073edc4e..3e189390f624 100644 --- a/pkg/ui/README.md +++ b/pkg/ui/README.md @@ -77,6 +77,14 @@ seem completely unrelated to your changes, try removing `yarn.installed` and Be sure to also commit modifications resulting from dependency changes, like updates to `package.json` and `yarn.lock`. +### Working with the `cluster-ui` dependency + +Many page-level components have been extracted into a +separate repository for sharing with other applications. +You can read all about this division in the [README for the +package](https://github.com/cockroachdb/ui/blob/master/packages/cluster-ui/README.md) +which describes a dev workflow that fits well with this package. + ### DLLs for speedy builds To improve Webpack compile times, we split the compile output into three From 745c4c135a149d3b32e1e8226815191ae45f1d1c Mon Sep 17 00:00:00 2001 From: RaduBerinde Date: Tue, 2 Mar 2021 14:31:08 -0800 Subject: [PATCH 2/7] Update CONTRIBUTING.md Release justification: non-production code change. Release note: None --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cd7defeae2b3..4f37296f8d29 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,7 +9,7 @@ For tech writers and docs enthusiasts | Help improve CockroachDB docs: [List of ## Contributor Guidelines -Our contributor guidelines are available on [the public wiki at **wiki.crdb.io**(https://wiki.crdb.io/wiki/spaces/CRDB/pages/73204033/Contributing+to+CockroachDB). +Our contributor guidelines are available on [the public wiki at **wiki.crdb.io**](https://wiki.crdb.io/wiki/spaces/CRDB/pages/73204033/Contributing+to+CockroachDB). At this location, we share our team guidelines and knowledge base regarding: From 514f2abec0854def1887d27230291059fe136acd Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Tue, 2 Mar 2021 18:06:45 -0500 Subject: [PATCH 3/7] backupccl: skip more TestProtectedTimestampSpanSelectionDuringBackup subtests Informs #57546. Release justification: non-production code change. Release note: None --- pkg/ccl/backupccl/backup_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 207c539e4987..f6d4ccfd217f 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -5868,6 +5868,7 @@ func TestProtectedTimestampSpanSelectionDuringBackup(t *testing.T) { }) t.Run("interleaved-spans", func(t *testing.T) { + skip.WithIssue(t, 57546, "flaky test") runner.Exec(t, "CREATE DATABASE test; USE test;") runner.Exec(t, "CREATE TABLE grandparent (a INT PRIMARY KEY, v BYTES, INDEX gpindex (v))") runner.Exec(t, "CREATE TABLE parent (a INT, b INT, v BYTES, "+ @@ -5886,6 +5887,7 @@ func TestProtectedTimestampSpanSelectionDuringBackup(t *testing.T) { }) t.Run("revs-span-merge", func(t *testing.T) { + skip.WithIssue(t, 57546, "flaky test") runner.Exec(t, "CREATE DATABASE test; USE test;") runner.Exec(t, "CREATE TABLE foo (k INT PRIMARY KEY, v BYTES, name STRING, "+ "INDEX baz(name), INDEX bar (v))") @@ -5919,6 +5921,7 @@ func TestProtectedTimestampSpanSelectionDuringBackup(t *testing.T) { }) t.Run("last-index-dropped", func(t *testing.T) { + skip.WithIssue(t, 57546, "flaky test") runner.Exec(t, "CREATE DATABASE test; USE test;") runner.Exec(t, "CREATE TABLE foo (k INT PRIMARY KEY, v BYTES, name STRING, INDEX baz(name))") runner.Exec(t, "CREATE TABLE foo2 (k INT PRIMARY KEY, v BYTES, name STRING, INDEX baz(name))") From 839de545e0b192b53ecae22dff6920ef12bc9821 Mon Sep 17 00:00:00 2001 From: Aayush Shah Date: Thu, 25 Feb 2021 01:58:07 -0500 Subject: [PATCH 4/7] kvserver, kvclient: allow Rangefeeds to run over non-voting replicas Prior to this commit, RangeFeeds only worked over voting replicas. This included followers as well as the leaseholder. This commit allows rangefeeds to run over non-voting replicas as well. Release justification: low risk high reward change to be able to fuel CDC streams using non-voting replicas Release note: None --- .../kvclient/kvcoord/dist_sender_rangefeed.go | 5 +- pkg/kv/kvserver/client_rangefeed_test.go | 60 +++++++++++++++++++ pkg/kv/kvserver/replica_rangefeed_test.go | 14 ++--- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go b/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go index db8acb309da8..8e4e4ced9e5e 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go @@ -229,8 +229,7 @@ func (ds *DistSender) singleRangeFeed( if ds.rpcContext != nil { latencyFn = ds.rpcContext.RemoteClocks.Latency } - // TODO(aayush): We should enable creating RangeFeeds on non-voting replicas. - replicas, err := NewReplicaSlice(ctx, ds.nodeDescs, desc, nil, OnlyPotentialLeaseholders) + replicas, err := NewReplicaSlice(ctx, ds.nodeDescs, desc, nil, AllExtantReplicas) if err != nil { return args.Timestamp, err } @@ -256,7 +255,7 @@ func (ds *DistSender) singleRangeFeed( log.VErrEventf(ctx, 2, "RPC error: %s", err) continue } - + log.VEventf(ctx, 3, "attempting to create a RangeFeed over replica %s", args.Replica) stream, err := client.RangeFeed(clientCtx, &args) if err != nil { log.VErrEventf(ctx, 2, "RPC error: %s", err) diff --git a/pkg/kv/kvserver/client_rangefeed_test.go b/pkg/kv/kvserver/client_rangefeed_test.go index 4741baa11b23..b8e1f2e8dbe0 100644 --- a/pkg/kv/kvserver/client_rangefeed_test.go +++ b/pkg/kv/kvserver/client_rangefeed_test.go @@ -13,6 +13,7 @@ package kvserver_test import ( "context" "testing" + "time" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/keys" @@ -27,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/stretchr/testify/require" ) @@ -182,3 +184,61 @@ func TestMergeOfRangeEventTableWhileRunningRangefeed(t *testing.T) { cancel() require.Regexp(t, context.Canceled.Error(), <-rangefeedErrChan) } + +func TestRangefeedIsRoutedToNonVoter(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + clusterArgs := aggressiveResolvedTimestampClusterArgs + // We want to manually add a non-voter to a range in this test, so disable + // the replicateQueue to prevent it from disrupting the test. + clusterArgs.ReplicationMode = base.ReplicationManual + // NB: setupClusterForClosedTSTesting sets a low closed timestamp target + // duration. + tc, _, desc := setupClusterForClosedTSTesting(ctx, t, testingTargetDuration, + testingCloseFraction, clusterArgs, "cttest", "kv") + defer tc.Stopper().Stop(ctx) + tc.AddNonVotersOrFatal(t, desc.StartKey.AsRawKey(), tc.Target(1)) + + db := tc.Server(1).DB() + ds := tc.Server(1).DistSenderI().(*kvcoord.DistSender) + _, err := tc.ServerConn(1).Exec("SET CLUSTER SETTING kv.rangefeed.enabled = true") + require.NoError(t, err) + + startTS := db.Clock().Now() + rangefeedCtx, rangefeedCancel := context.WithCancel(ctx) + rangefeedCtx, getRec, cancel := tracing.ContextWithRecordingSpan(rangefeedCtx, + tracing.NewTracer(), + "rangefeed over non-voter") + defer cancel() + + // Do a read on the range to make sure that the dist sender learns about the + // latest state of the range (with the new non-voter). + _, err = db.Get(ctx, desc.StartKey.AsRawKey()) + require.NoError(t, err) + + rangefeedErrChan := make(chan error, 1) + eventCh := make(chan *roachpb.RangeFeedEvent, 1000) + go func() { + rangefeedErrChan <- ds.RangeFeed( + rangefeedCtx, + desc.RSpan().AsRawSpanWithNoLocals(), + startTS, + false, /* withDiff */ + eventCh, + ) + }() + + // Wait for an event to ensure that the rangefeed is set up. + select { + case <-eventCh: + case err := <-rangefeedErrChan: + t.Fatalf("rangefeed failed with %s", err) + case <-time.After(60 * time.Second): + t.Fatalf("rangefeed initialization took too long") + } + rangefeedCancel() + require.Regexp(t, "context canceled", <-rangefeedErrChan) + require.Regexp(t, "attempting to create a RangeFeed over replica.*2NON_VOTER", getRec().String()) +} diff --git a/pkg/kv/kvserver/replica_rangefeed_test.go b/pkg/kv/kvserver/replica_rangefeed_test.go index de435bd8712b..b51af9d1b9a9 100644 --- a/pkg/kv/kvserver/replica_rangefeed_test.go +++ b/pkg/kv/kvserver/replica_rangefeed_test.go @@ -88,7 +88,7 @@ func TestReplicaRangefeed(t *testing.T) { defer log.Scope(t).Close(t) ctx := context.Background() - const numNodes = 3 + const numNodes = 5 args := base.TestClusterArgs{ ReplicationMode: base.ReplicationManual, ServerArgsPerNode: make(map[int]base.TestServerArgs, numNodes), @@ -116,6 +116,7 @@ func TestReplicaRangefeed(t *testing.T) { startKey := []byte("a") tc.SplitRangeOrFatal(t, startKey) tc.AddVotersOrFatal(t, startKey, tc.Target(1), tc.Target(2)) + tc.AddNonVotersOrFatal(t, startKey, tc.Target(3), tc.Target(4)) if pErr := tc.WaitForVoters(startKey, tc.Target(1), tc.Target(2)); pErr != nil { t.Fatalf("Unexpected error waiting for replication: %v", pErr) } @@ -128,13 +129,12 @@ func TestReplicaRangefeed(t *testing.T) { if _, pErr := kv.SendWrappedWith(ctx, db, roachpb.Header{Timestamp: ts1}, incArgs); pErr != nil { t.Fatal(pErr) } - tc.WaitForValues(t, roachpb.Key("b"), []int64{9, 9, 9}) + tc.WaitForValues(t, roachpb.Key("b"), []int64{9, 9, 9, 9, 9}) - replNum := 3 - streams := make([]*testStream, replNum) - streamErrC := make(chan *roachpb.Error, replNum) + streams := make([]*testStream, numNodes) + streamErrC := make(chan *roachpb.Error, numNodes) rangefeedSpan := roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("z")} - for i := 0; i < replNum; i++ { + for i := 0; i < numNodes; i++ { stream := newTestStream() streams[i] = stream ts := tc.Servers[i] @@ -308,7 +308,7 @@ func TestReplicaRangefeed(t *testing.T) { } testutils.SucceedsSoon(t, func() error { - for i := 0; i < replNum; i++ { + for i := 0; i < numNodes; i++ { ts := tc.Servers[i] store, pErr := ts.Stores().GetStore(ts.GetFirstStoreID()) if pErr != nil { From 69d4b67e8979bac360ead2ddb477fc234da3869b Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 4 Mar 2021 07:15:16 +1100 Subject: [PATCH 5/7] roachtest: install GEOS libraries for activerecord tests Release justification: non-production code change Release note: None --- pkg/cmd/roachtest/activerecord.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/roachtest/activerecord.go b/pkg/cmd/roachtest/activerecord.go index ebd980ca914b..8a6315f1df70 100644 --- a/pkg/cmd/roachtest/activerecord.go +++ b/pkg/cmd/roachtest/activerecord.go @@ -37,6 +37,9 @@ func registerActiveRecord(r *testRegistry) { node := c.Node(1) t.Status("setting up cockroach") c.Put(ctx, cockroach, "./cockroach", c.All()) + if err := c.PutLibraries(ctx, "./lib"); err != nil { + t.Fatal(err) + } c.Start(ctx, t, c.All()) version, err := fetchCockroachVersion(ctx, c, node[0]) From c5df5ecb7d070087d456bff7cb45432863584b8f Mon Sep 17 00:00:00 2001 From: Eric Wang Date: Wed, 3 Mar 2021 16:29:04 -0500 Subject: [PATCH 6/7] sql: use correct FuncExpr when encoding sequences Previously, when encoding sequences by swapping sequence names for IDs, we were always wrapping the sequence in a nextval func. This is incorrect, and instead we should wrap the sequence in whatever function it came in before this encoding. This patch makes this change. Release justification: bug fix for new functionality Release note (bug fix): use correct FuncExpr when encoding sequences. --- .../testdata/logic_test/sequences_regclass | 43 +++++++++++++------ pkg/util/sequence/sequence.go | 2 +- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/sequences_regclass b/pkg/sql/logictest/testdata/logic_test/sequences_regclass index 79f58d361c2d..6273a828c713 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences_regclass +++ b/pkg/sql/logictest/testdata/logic_test/sequences_regclass @@ -29,7 +29,10 @@ statement ok ALTER TABLE foo ADD COLUMN l INT NOT NULL statement ok -ALTER TABLE FOO ALTER COLUMN l SET DEFAULT nextval('test_seq2') +ALTER TABLE FOO ALTER COLUMN l SET DEFAULT currval('test_seq2') + +statement ok +SELECT nextval('test_seq2') query TT SHOW CREATE TABLE foo @@ -38,7 +41,7 @@ foo CREATE TABLE public.foo ( i INT8 NOT NULL DEFAULT nextval('test.public.foo_i_seq':::STRING::REGCLASS), j INT8 NOT NULL DEFAULT nextval('test.public.test_seq':::STRING::REGCLASS), k INT8 NOT NULL DEFAULT nextval('test.public.foo_k_seq':::STRING::REGCLASS), - l INT8 NOT NULL DEFAULT nextval('test.public.test_seq2':::STRING::REGCLASS), + l INT8 NOT NULL DEFAULT currval('test.public.test_seq2':::STRING::REGCLASS), CONSTRAINT "primary" PRIMARY KEY (i ASC), FAMILY "primary" (i, j, k, l) ) @@ -103,10 +106,13 @@ CREATE SEQUENCE s2 statement ok CREATE TABLE bar ( i SERIAL PRIMARY KEY, - j INT NOT NULL DEFAULT nextval($s1_id::regclass), + j INT NOT NULL DEFAULT currval($s1_id::regclass), k INT NOT NULL DEFAULT nextval('s2'), FAMILY (i, j, k)) +statement ok +SELECT nextval($s1_id::regclass) + statement ok INSERT INTO bar VALUES (default, default, default) @@ -125,7 +131,7 @@ SHOW CREATE TABLE bar ---- bar CREATE TABLE public.bar ( i INT8 NOT NULL DEFAULT nextval('test.public.new_bar_i_seq':::STRING::REGCLASS), - j INT8 NOT NULL DEFAULT nextval('test.public.new_s1':::STRING::REGCLASS), + j INT8 NOT NULL DEFAULT currval('test.public.new_s1':::STRING::REGCLASS), k INT8 NOT NULL DEFAULT nextval('test.public.new_s2':::STRING::REGCLASS), CONSTRAINT "primary" PRIMARY KEY (i ASC), FAMILY fam_0_i_j_k (i, j, k) @@ -139,7 +145,7 @@ query III SELECT i, j, k FROM bar ORDER BY i, j, k ---- 1 1 1 -2 2 2 +2 1 2 # Test that databases with sequences can be renamed, even if they are @@ -164,9 +170,12 @@ CREATE SEQUENCE other_db.s2 statement ok CREATE TABLE other_db.t ( i INT NOT NULL DEFAULT nextval($s_id::regclass), - j INT NOT NULL DEFAULT nextval('other_db.public.s2'), + j INT NOT NULL DEFAULT currval('other_db.public.s2'), FAMILY (i, j)) +statement ok +SELECT nextval('other_db.public.s2') + statement ok INSERT INTO other_db.t VALUES (default, default) @@ -179,7 +188,7 @@ SHOW CREATE TABLE new_other_db.t ---- new_other_db.public.t CREATE TABLE public.t ( i INT8 NOT NULL DEFAULT nextval('new_other_db.public.s':::STRING::REGCLASS), - j INT8 NOT NULL DEFAULT nextval('new_other_db.public.s2':::STRING::REGCLASS), + j INT8 NOT NULL DEFAULT currval('new_other_db.public.s2':::STRING::REGCLASS), rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(), CONSTRAINT "primary" PRIMARY KEY (rowid ASC), FAMILY fam_0_i_j_rowid (i, j, rowid) @@ -193,7 +202,7 @@ query II SELECT i, j FROM new_other_db.t ORDER BY i, j ---- 1 1 -2 2 +2 1 # Test that sequences can change schemas even if they're referenced @@ -219,9 +228,12 @@ statement ok CREATE TABLE tb ( i SERIAL PRIMARY KEY, j INT NOT NULL DEFAULT nextval($sc_s1_id::regclass), - k INT NOT NULL DEFAULT nextval('test.public.sc_s2'), + k INT NOT NULL DEFAULT currval('test.public.sc_s2'), FAMILY (i, j, k)) +statement ok +SELECT nextval('test.public.sc_s2') + statement ok INSERT INTO tb VALUES (default, default, default) @@ -241,7 +253,7 @@ SHOW CREATE TABLE tb tb CREATE TABLE public.tb ( i INT8 NOT NULL DEFAULT nextval('test.test_schema.tb_i_seq':::STRING::REGCLASS), j INT8 NOT NULL DEFAULT nextval('test.test_schema.sc_s1':::STRING::REGCLASS), - k INT8 NOT NULL DEFAULT nextval('test.test_schema.sc_s2':::STRING::REGCLASS), + k INT8 NOT NULL DEFAULT currval('test.test_schema.sc_s2':::STRING::REGCLASS), CONSTRAINT "primary" PRIMARY KEY (i ASC), FAMILY fam_0_i_j_k (i, j, k) ) @@ -254,7 +266,7 @@ query III SELECT i, j, k FROM tb ORDER BY i, j, k ---- 1 1 1 -2 2 2 +2 2 1 # Test that sequences can have their schemas renamed even if @@ -277,9 +289,12 @@ statement ok CREATE TABLE foo ( i SERIAL PRIMARY KEY, j INT NOT NULL DEFAULT nextval($s3_id::regclass), - k INT NOT NULL DEFAULT nextval('test.test_schema.s4'), + k INT NOT NULL DEFAULT currval('test.test_schema.s4'), FAMILY (i, j, k)) +statement ok +SELECT nextval('test.test_schema.s4') + statement ok INSERT INTO foo VALUES (default, default, default) @@ -293,7 +308,7 @@ SHOW CREATE TABLE new_test_schema.foo new_test_schema.foo CREATE TABLE new_test_schema.foo ( i INT8 NOT NULL DEFAULT nextval('test.new_test_schema.foo_i_seq':::STRING::REGCLASS), j INT8 NOT NULL DEFAULT nextval('test.new_test_schema.s3':::STRING::REGCLASS), - k INT8 NOT NULL DEFAULT nextval('test.new_test_schema.s4':::STRING::REGCLASS), + k INT8 NOT NULL DEFAULT currval('test.new_test_schema.s4':::STRING::REGCLASS), CONSTRAINT "primary" PRIMARY KEY (i ASC), FAMILY fam_0_i_j_k (i, j, k) ) @@ -306,7 +321,7 @@ query III SELECT i, j, k FROM new_test_schema.foo ORDER BY i, j, k ---- 1 1 1 -2 2 2 +2 2 1 statement ok SET SCHEMA public diff --git a/pkg/util/sequence/sequence.go b/pkg/util/sequence/sequence.go index 685dc7faf163..e577badda914 100644 --- a/pkg/util/sequence/sequence.go +++ b/pkg/util/sequence/sequence.go @@ -143,7 +143,7 @@ func ReplaceSequenceNamesWithIDs( return true, expr, nil } return false, &tree.FuncExpr{ - Func: tree.WrapFunction("nextval"), + Func: t.Func, Exprs: tree.Exprs{ &tree.AnnotateTypeExpr{ Type: types.RegClass, From 03319af5b746d05e228426b85bdcd0956e21a118 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Fri, 26 Feb 2021 14:41:23 -0500 Subject: [PATCH 7/7] sql: alter primary key is not idempotent Fixes: #59307 Previously, issuing an alter primary key with the exact same definition as the current primary key would cause expensive indexes to be recreated even when there was no logical change. There was overhead involved in creating these indexes and as a side effect existing indexes for the primary key would also get needlessly renamed. To address this, this patch uses the AST node for alter primary key to determine if the requested change is logically the same before any operation is executed. If its logically the same then it becomes a no-op. Release justification: This is a low risk change with good benefit to the user base by reducing extra indexes getting made and rename operations. Release note (bug fix): Alter primary key was not idempotent, so logical equivalent changes to primary keys would unnecessarily create new indexes. --- pkg/ccl/backupccl/backup_test.go | 3 +- pkg/sql/alter_primary_key.go | 109 ++++++++++++ .../testdata/logic_test/alter_primary_key | 164 ++++++++++++++++++ 3 files changed, 274 insertions(+), 2 deletions(-) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 207c539e4987..84548e950a56 100644 --- a/pkg/ccl/backupccl/backup_test.go +++ b/pkg/ccl/backupccl/backup_test.go @@ -828,7 +828,6 @@ func TestBackupRestoreNegativePrimaryKey(t *testing.T) { backupAndRestore(ctx, t, tc, []string{LocalFoo}, []string{LocalFoo}, numAccounts) sqlDB.Exec(t, `CREATE UNIQUE INDEX id2 ON data.bank (id)`) - sqlDB.Exec(t, `ALTER TABLE data.bank ALTER PRIMARY KEY USING COLUMNS(id)`) var unused string var exportedRows, exportedIndexEntries int @@ -838,7 +837,7 @@ func TestBackupRestoreNegativePrimaryKey(t *testing.T) { if exportedRows != numAccounts { t.Fatalf("expected %d rows, got %d", numAccounts, exportedRows) } - expectedIndexEntries := numAccounts * 3 // old PK, new and old secondary idx + expectedIndexEntries := numAccounts * 2 // Indexes id2 and balance_idx if exportedIndexEntries != expectedIndexEntries { t.Fatalf("expected %d index entries, got %d", expectedIndexEntries, exportedIndexEntries) } diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index 6687fb62b9b4..0ad8282b41bf 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" @@ -145,6 +146,19 @@ func (p *planner) AlterPrimaryKey( ) } + // Validate if the end result is the same as the current + // primary index, which would mean nothing needs to be modified + // here. + { + requiresIndexChange, err := p.shouldCreateIndexes(ctx, tableDesc, alterPKNode, alterPrimaryKeyLocalitySwap) + if err != nil { + return err + } + if !requiresIndexChange { + return nil + } + } + nameExists := func(name string) bool { _, err := tableDesc.FindIndexWithName(name) return err == nil @@ -508,6 +522,101 @@ func (p *planner) AlterPrimaryKey( return nil } +// Given the current table descriptor and the new primary keys +// index descriptor this function determines if the two are +// equivalent and if any index creation operations are needed +// by comparing properties. +func (p *planner) shouldCreateIndexes( + ctx context.Context, + desc *tabledesc.Mutable, + alterPKNode *tree.AlterTableAlterPrimaryKey, + alterPrimaryKeyLocalitySwap *alterPrimaryKeyLocalitySwap, +) (requiresIndexChange bool, err error) { + oldPK := desc.GetPrimaryIndex() + + // Validate if basic properties between the two match. + if len(oldPK.IndexDesc().ColumnIDs) != len(alterPKNode.Columns) || + oldPK.IsSharded() != (alterPKNode.Sharded != nil) || + oldPK.IsInterleaved() != (alterPKNode.Interleave != nil) { + return true, nil + } + + // Validate if sharding properties are the same. + if alterPKNode.Sharded != nil { + shardBuckets, err := tabledesc.EvalShardBucketCount(ctx, &p.semaCtx, p.EvalContext(), alterPKNode.Sharded.ShardBuckets) + if err != nil { + return true, err + } + if oldPK.IndexDesc().Sharded.ShardBuckets != shardBuckets { + return true, nil + } + } + + // Validate if interleaving properties match, + // specifically the parent table, and the index + // involved. + if alterPKNode.Interleave != nil { + parentTable, err := resolver.ResolveExistingTableObject( + ctx, p, &alterPKNode.Interleave.Parent, tree.ObjectLookupFlagsWithRequiredTableKind(tree.ResolveRequireTableDesc), + ) + if err != nil { + return true, err + } + + ancestors := oldPK.IndexDesc().Interleave.Ancestors + if len(ancestors) == 0 { + return true, nil + } + if ancestors[len(ancestors)-1].TableID != + parentTable.GetID() { + return true, nil + } + if ancestors[len(ancestors)-1].IndexID != + parentTable.GetPrimaryIndexID() { + return true, nil + } + } + + // If the old primary key is dropped, then recreation + // is required. + if oldPK.IndexDesc().Disabled { + return true, nil + } + + // Validate the columns on the indexes + for idx, elem := range alterPKNode.Columns { + col, err := desc.FindColumnWithName(elem.Column) + if err != nil { + return true, err + } + + if col.GetID() != oldPK.IndexDesc().ColumnIDs[idx] { + return true, nil + } + if (elem.Direction == tree.Ascending && + oldPK.IndexDesc().ColumnDirections[idx] != descpb.IndexDescriptor_ASC) || + (elem.Direction == tree.Descending && + oldPK.IndexDesc().ColumnDirections[idx] != descpb.IndexDescriptor_DESC) { + return true, nil + } + } + + // Check partitioning changes based on primary key locality, + // either the config changes, or the region column is changed + // then recreate indexes. + if alterPrimaryKeyLocalitySwap != nil { + localitySwapConfig := alterPrimaryKeyLocalitySwap.localityConfigSwap + if !localitySwapConfig.NewLocalityConfig.Equal(localitySwapConfig.OldLocalityConfig) { + return true, nil + } + if localitySwapConfig.NewRegionalByRowColumnID != nil && + *localitySwapConfig.NewRegionalByRowColumnID != oldPK.IndexDesc().ColumnIDs[0] { + return true, nil + } + } + return false, nil +} + // We only recreate the old primary key of the table as a unique secondary // index if: // * The table has a primary key (no DROP PRIMARY KEY statements have diff --git a/pkg/sql/logictest/testdata/logic_test/alter_primary_key b/pkg/sql/logictest/testdata/logic_test/alter_primary_key index 0f3f0f7909ad..6fa66b1f19e7 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_primary_key +++ b/pkg/sql/logictest/testdata/logic_test/alter_primary_key @@ -1240,3 +1240,167 @@ ALTER TABLE t54629 ALTER PRIMARY KEY USING COLUMNS (c); DROP INDEX t54629_a_key CASCADE; INSERT INTO t54629 VALUES (1, 1); DELETE FROM t54629 WHERE c = 1; + +# Validate ALTER ADD PRIMARY KEY idempotence for #59307 +statement ok +create table t1(id integer not null, id2 integer not null, name varchar(32)); + +query TTT +select index_name,column_name,direction from [show indexes from t1] where index_name like 'primary%'; +---- +primary rowid ASC + +statement ok +alter table t1 alter primary key using columns(id, id2); + + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id ASC +primary id2 ASC + + +statement ok +alter table t1 alter primary key using columns(id, id2); + + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id ASC +primary id2 ASC + +# Validate drop and recreate +statement ok +alter table t1 drop constraint "primary", alter primary key using columns(id, id2); + + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id ASC +primary id2 ASC + +statement ok +alter table t1 alter primary key using columns(id); + + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id ASC +t1_id_id2_key id ASC +t1_id_id2_key id2 ASC + +statement ok +alter table t1 alter primary key using columns(id desc); + + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id DESC +t1_id_id2_key id ASC +t1_id_id2_key id2 ASC +t1_id_key id ASC + + +statement ok +alter table t1 alter primary key using columns(id desc); + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id DESC +t1_id_id2_key id ASC +t1_id_id2_key id2 ASC +t1_id_key id ASC + +statement ok +alter table t1 alter primary key using columns(id desc); + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary id DESC +t1_id_id2_key id ASC +t1_id_id2_key id2 ASC +t1_id_key id ASC + +statement ok +alter table t1 alter primary key using columns(id) USING HASH WITH BUCKET_COUNT = 10 + +query TTT +select index_name,column_name,direction from [show indexes from t1]; +---- +primary crdb_internal_id_shard_10 ASC +primary id ASC +t1_id_key1 id DESC +t1_id_key1 crdb_internal_id_shard_10 ASC +t1_id_id2_key id ASC +t1_id_id2_key id2 ASC +t1_id_id2_key crdb_internal_id_shard_10 ASC +t1_id_key id ASC +t1_id_key crdb_internal_id_shard_10 ASC + +statement ok +CREATE TABLE parent3 (x INT, y INT, PRIMARY KEY (x, y), FAMILY (x, y)); + +statement ok +CREATE TABLE child3 (x INT PRIMARY KEY, y INT NOT NULL, z INT NOT NULL, FAMILY (x, y, z)); + +statement ok +INSERT INTO parent3 VALUES (1, 2), (4, 5); + +statement ok +INSERT INTO child3 VALUES (1, 2, 3), (4, 5, 6); + +statement ok +ALTER TABLE child3 ALTER PRIMARY KEY USING COLUMNS (x, y, z) INTERLEAVE IN PARENT parent3(x, y) + +query TTT +select index_name,column_name,direction from [show indexes from child3]; +---- +primary x ASC +primary y ASC +primary z ASC +child3_x_key x ASC +child3_x_key y ASC +child3_x_key z ASC + +statement ok +ALTER TABLE child3 ALTER PRIMARY KEY USING COLUMNS (x, y, z) INTERLEAVE IN PARENT parent3(x, y) + +query TTT +select index_name,column_name,direction from [show indexes from child3]; +---- +primary x ASC +primary y ASC +primary z ASC +child3_x_key x ASC +child3_x_key y ASC +child3_x_key z ASC + +statement ok +ALTER TABLE child3 ALTER PRIMARY KEY USING COLUMNS (x, y) INTERLEAVE IN PARENT parent3(x, y) + +query TTT +select index_name,column_name,direction from [show indexes from child3]; +---- +primary x ASC +primary y ASC +child3_x_key x ASC +child3_x_key y ASC +child3_x_y_z_key x ASC +child3_x_y_z_key y ASC +child3_x_y_z_key z ASC + +statement ok +drop table t1; + +statement ok +drop table child3; + +statement ok +drop table parent3;