From fc5133981238ecd1ae6e3645d49c9249cd119425 Mon Sep 17 00:00:00 2001 From: Rohan Yadav Date: Wed, 29 Jan 2020 11:20:14 -0500 Subject: [PATCH] sql: bugfixes around writing the old primary key in pk changes This PR fixes two bugs: * The logic for when to rewrite the old primary key was broken resulting in the old primary key not being rewritten in many cases. * The old primary key being created was not properly dealing with its dangling interleave information. Release note: None --- pkg/sql/alter_table.go | 14 ++++++------- .../testdata/logic_test/alter_primary_key | 21 ++++++++++++++++++- pkg/sql/schema_changer_test.go | 2 +- pkg/sql/sqlbase/structured.go | 14 +++++++++++++ 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 6c7d252eaef6..a251ff17b20b 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -397,17 +397,15 @@ func (n *alterTableNode) startExec(params runParams) error { // TODO (rohany): gate this behind a flag so it doesn't happen all the time. // Create a new index that indexes everything the old primary index does, but doesn't store anything. - // TODO (rohany): is there an easier way of checking if the existing primary index was the - // automatically created one? - if len(n.tableDesc.PrimaryIndex.ColumnNames) == 1 && n.tableDesc.PrimaryIndex.ColumnNames[0] != "rowid" { + if !n.tableDesc.IsPrimaryIndexDefaultRowID() { oldPrimaryIndexCopy := protoutil.Clone(&n.tableDesc.PrimaryIndex).(*sqlbase.IndexDescriptor) - name := generateUniqueConstraintName( - "old_primary_key", - nameExists, - ) - oldPrimaryIndexCopy.Name = name + // Clear the name of the index so that it gets generated by AllocateIDs. + oldPrimaryIndexCopy.Name = "" oldPrimaryIndexCopy.StoreColumnIDs = nil oldPrimaryIndexCopy.StoreColumnNames = nil + // Make the copy of the old primary index not-interleaved. This decision + // can be revisited based on user experience. + oldPrimaryIndexCopy.Interleave = sqlbase.InterleaveDescriptor{} if err := addIndexMutationWithSpecificPrimaryKey(n.tableDesc, oldPrimaryIndexCopy, newPrimaryIndexDesc); err != nil { return err } diff --git a/pkg/sql/logictest/testdata/logic_test/alter_primary_key b/pkg/sql/logictest/testdata/logic_test/alter_primary_key index a542fe022cad..6c44f93f94f3 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_primary_key +++ b/pkg/sql/logictest/testdata/logic_test/alter_primary_key @@ -97,7 +97,7 @@ child CREATE TABLE child ( y INT8 NOT NULL, z INT8 NOT NULL, CONSTRAINT "primary" PRIMARY KEY (x ASC, y ASC, z ASC), - UNIQUE INDEX old_primary_key (x ASC), + UNIQUE INDEX child_x_key (x ASC), FAMILY fam_0_x_y_z (x, y, z) ) INTERLEAVE IN PARENT parent (x, y) @@ -151,6 +151,7 @@ child CREATE TABLE child ( y INT8 NOT NULL, z INT8 NOT NULL, CONSTRAINT "primary" PRIMARY KEY (y ASC, z ASC), + UNIQUE INDEX child_x_y_z_key (x ASC, y ASC, z ASC), FAMILY fam_0_x_y_z (x, y, z) ) @@ -194,6 +195,7 @@ child CREATE TABLE child ( z INT8 NOT NULL, w INT8 NULL, CONSTRAINT "primary" PRIMARY KEY (x ASC, y ASC, z ASC), + UNIQUE INDEX child_x_y_key (x ASC, y ASC), INDEX i (x ASC, w ASC) INTERLEAVE IN PARENT parent (x), FAMILY fam_0_x_y_z_w (x, y, z, w) ) INTERLEAVE IN PARENT parent (x) @@ -281,3 +283,20 @@ INSERT INTO t1 VALUES (100, 100, 100, 100) statement error insert on table "t4" violates foreign key constraint "fk3" INSERT INTO t4 VALUES (101) + +# Ensure that we still rewrite a primary index if the index column has name "rowid". +statement ok +DROP TABLE IF EXISTS t; +CREATE TABLE t (rowid INT PRIMARY KEY, y INT NOT NULL, FAMILY (rowid, y)); +ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (y) + +query TT +SHOW CREATE t +---- +t CREATE TABLE t ( + rowid INT8 NOT NULL, + y INT8 NOT NULL, + CONSTRAINT "primary" PRIMARY KEY (y ASC), + UNIQUE INDEX t_rowid_key (rowid ASC), + FAMILY fam_0_rowid_y (rowid, y) +) diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index 35f2b3c03cf1..31ce21c5e930 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -2322,7 +2322,7 @@ INSERT INTO t.test VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3); y INT8 NOT NULL, z INT8 NULL, CONSTRAINT "primary" PRIMARY KEY (y ASC), - UNIQUE INDEX old_primary_key (x ASC), + UNIQUE INDEX test_x_key (x ASC), INDEX i (z ASC), FAMILY "primary" (x, y, z) )` diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index 0bd5563f3aa9..69aaf75f5d28 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -2808,6 +2808,20 @@ func (desc *TableDescriptor) IsInterleaved() bool { return false } +// IsPrimaryIndexDefaultRowID returns whether or not the table's primary +// index is the default primary key on the hidden rowid column. +func (desc *TableDescriptor) IsPrimaryIndexDefaultRowID() bool { + if len(desc.PrimaryIndex.ColumnIDs) != 1 { + return false + } + col, err := desc.FindColumnByID(desc.PrimaryIndex.ColumnIDs[0]) + if err != nil { + // Should never be in this case. + panic(err) + } + return col.Hidden && col.Name == "rowid" +} + // MakeMutationComplete updates the descriptor upon completion of a mutation. // There are three Validity types for the mutations: // Validated - The constraint has already been added and validated, should