Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: cannot drop columns after ALTER PRIMARY KEY #49079

Closed
RichardJCai opened this issue May 14, 2020 · 2 comments · Fixed by #49081
Closed

sql: cannot drop columns after ALTER PRIMARY KEY #49079

RichardJCai opened this issue May 14, 2020 · 2 comments · Fixed by #49081
Assignees
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@RichardJCai
Copy link
Contributor

This stems from not being able to drop columns that are stored in the PK.
In alter_table.go, we check if the column is stored in the pk, if it is, it disallows dropping the column. For PK we should only be checking if the column is indexed.

 n.tableDesc.PrimaryIndex.ContainsColumnID(colToDrop.ID) {
	return pgerror.Newf(pgcode.InvalidColumnReference,
	"column %q is referenced by the primary key", colToDrop.Name)
}

// ContainsColumnID returns true if the index descriptor contains the specified
// column ID either in its explicit column IDs, the extra column IDs, or the
// stored column IDs.
func (desc *IndexDescriptor) ContainsColumnID(colID ColumnID) bool {
	return desc.RunOverAllColumns(func(id ColumnID) error {
		if id == colID {
			return returnTruePseudoError
		}
		return nil
	}) != nil
}

Example: Cannot drop any columns after running ALTER PRIMARY KEY since the columns become explicitly stored.

[email protected]:51308/movr> create table t(x int);
CREATE TABLE
Time: 3.229ms
[email protected]:51308/movr> alter table t alter primary key using columns (rowid);
NOTICE: primary key changes are finalized asynchronously; further schema changes on this table may be restricted until the job completes
ALTER TABLE
Time: 89.82ms
[email protected]:51308/movr> set sql_safe_updates = false;
SET
Time: 249µs
[email protected]:51308/movr> alter table t drop column x;
ERROR: column "x" is referenced by the primary key
SQLSTATE: 42P10
[email protected]:51308/movr>
@blathers-crl
Copy link

blathers-crl bot commented May 14, 2020

Hi @RichardJCai, please add a C-ategory label to your issue. Check out the label system docs.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@RichardJCai
Copy link
Contributor Author

@rohany

@RichardJCai RichardJCai added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label May 14, 2020
@rohany rohany self-assigned this May 14, 2020
craig bot pushed a commit that referenced this issue May 14, 2020
48778: sql: support tenant creation and deletion, track in system.tenants table r=nvanbenschoten a=nvanbenschoten

Most of #47904.

This commit implements the initial structure for tenant creation and deletion. It does so by introducing a new system table to track tenants in a multi-tenant cluster and two new builtin functions to manipulate this table and the overall multi-tenant state.

The change starts by introducing a new `system.tenants` table with the following schema:
```
CREATE TABLE system.tenants (
    id     INT8 NOT NULL PRIMARY KEY,
    active BOOL NOT NULL DEFAULT true,
    info   BYTES
);
```

The `id` column is self-explanatory. The `active` column is used to coordinate tenant destruction in an asynchronous job. The `info` column is an opaque byte slice to allow users to associate arbitrary information with specific tenants. I don't know exactly how this third field will be used (mapping tenants back to CockroachCloud user IDs?), but it seems like a good idea to add some flexibility since we do intend to eventually expose this externally.

The table is given an ID of 8, which means that it falls within the "system config span". I believe this is ok, because the entire concept of the "system config span" should be going away with #47150. The table is also only exposed to the system-tenant and is never created for secondary tenants.

The change then introduces two new builtin functions: `crdb_internal.create_tenant` and `crdb_internal.destroy_tenant`. These do what you would expect - creating and destroying tenant keyspaces, along with updating metadata in system.tenants.

48830: opt: correctly ignore NULL in array indirection r=mjibson a=mjibson

Fixes #48826

Release note (bug fix): fix an error that could occur when using NULL
in some array indirections.

49081: sql: fix bug where columns couldn't be dropped after a pk change r=rohany a=rohany

Fixes #49079.

Release note (bug fix): Fix a bug where columns of a table could not
be dropped after a primary key change.

49082: kv: remove special handling of MergeTrigger in batcheval r=nvanbenschoten a=nvanbenschoten

Discovered with @ajwerner while investigating #48770.

This commit removes the special treatment that we used to give
the MergeTrigger - running it before local lock cleanup. This
was originally added back when the MergeTrigger held a snapshot
of the RHS's data, including intents that needed to be resolved
synchronously with the trigger. This snapshot was removed shortly
afterwards in bb7426a, but the special handling during batch
evaluation persisted, leading to confusing code.

While here, avoid copying around the transaction Result on the
hot path where there are no commit triggers. This was not free,
as is demonstrated by the following benchmark:
```
func BenchmarkMergeTxnCommitResult(b *testing.B) {
	locks := []roachpb.Span{{Key: roachpb.Key("a")}, {Key: roachpb.Key("b")}}
	txn := &roachpb.Transaction{LockSpans: locks}
	txnResult := result.FromEndTxn(txn, false /* alwaysReturn */, false)
	txnResult.Local.UpdatedTxns = []*roachpb.Transaction{txn}
	txnResult.Local.ResolvedLocks = roachpb.AsLockUpdates(txn, locks)

	for i := 0; i < b.N; i++ {
		var res result.Result
		if err := res.MergeAndDestroy(txnResult); err != nil {
			b.Fatal(err)
		}
	}
}

// BenchmarkMergeTxnCommitResult-16   12777922   86.2 ns/op   0 B/op   0 allocs/op
```

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Matt Jibson <[email protected]>
Co-authored-by: Rohan Yadav <[email protected]>
@craig craig bot closed this as completed in 71631ec May 14, 2020
rohany added a commit to rohany/cockroach that referenced this issue May 14, 2020
Fixes cockroachdb#49079.

Release note (bug fix): Fix a bug where columns of a table could not
be dropped after a primary key change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants