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: diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 207c539e4987..a57503e274b0 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) } @@ -5868,6 +5867,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 +5886,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 +5920,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))") 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]) 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 { diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index d05310ad7ba4..7500f09aef9c 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" @@ -148,6 +149,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 @@ -511,6 +525,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; 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/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 b16362035697..53b9148f0aec 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 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,