sql: implement the `DROP TYPE` command by rohany · Pull Request #51240 · cockroachdb/cockroach

rohany · 2020-07-09T19:48:46Z

This PR implements the DROP TYPE command without support for
CASCADE. It lays the foundation of the dependency management between
types and tables that use them. With this, we can drop types when a
database is dropped.

Fixes #48363.

Release note: None

cockroach-teamcity · 2020-07-09T19:48:55Z

This change is

rohany · 2020-07-09T21:58:31Z

This looks like it needs the import changes in #51149.

pkg/sql/drop_type.go

ajwerner

Mostly lgtm just some commentary

Reviewed 3 of 17 files at r1, 1 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @RichardJCai, and @rohany)

pkg/sql/schema_changer.go, line 470 at r2 (raw file):

		// by the table.
		// TODO (rohany): Do we need to delay the back reference removal until
		//  the schema change job execution? Doing it here leads means that we

leads means but more to the point, I think we need to sort this out.

Why does doing it here mean that? Can't we be robust to the concurrent operations?

pkg/sql/schema_changer.go, line 1037 at r2 (raw file):

		if !scTable.Dropped() {
			newReferencedTypeIDs, err := scTable.GetAllReferencedTypeIDs(func(id sqlbase.ID) (*TypeDescriptor, error) {
				return descs[id].(*sqlbase.MutableTypeDescriptor).TypeDesc(), nil

Are you worried about panics here?

pkg/sql/type_change.go, line 161 at r2 (raw file):

	}

	// TODO (rohany): Not sure if this needs to be done before or after

I think after draining.

pkg/sql/type_change.go, line 166 at r2 (raw file):

	if typeDesc.State == sqlbase.TypeDescriptor_DROP {
		if err := t.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
			// TODO (rohany): Do we need to set the systemConfigTrigger here?

Nope. We need to do that when creating a split point (table) or changing zone configs. Working on an RFC on that topic right now.

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, and @RichardJCai)

pkg/sql/schema_changer.go, line 470 at r2 (raw file):

Previously, ajwerner wrote…

leads means but more to the point, I think we need to sort this out.

Why does doing it here mean that? Can't we be robust to the concurrent operations?

I meant doing the back referenced removal in the transaction that commits the drop table vs removing the back reference in the schema changer. If we delay to the schema changer then we can't do

BEGIN;
DROP TABLE t;
DROP TYPE <t used to use>
END;

I'm not sure that there is a problem with doing it in the drop table txn.

pkg/sql/schema_changer.go, line 1037 at r2 (raw file):

Previously, ajwerner wrote…

Are you worried about panics here?

No, because the descs map is a MutableDescriptor map

pkg/sql/type_change.go, line 161 at r2 (raw file):

Previously, ajwerner wrote…

I think after draining.

Thanks

pkg/sql/type_change.go, line 166 at r2 (raw file):

Previously, ajwerner wrote…

Nope. We need to do that when creating a split point (table) or changing zone configs. Working on an RFC on that topic right now.

Thanks

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @RichardJCai, and @rohany)

pkg/sql/schema_changer.go, line 470 at r2 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I meant doing the back referenced removal in the transaction that commits the drop table vs removing the back reference in the schema changer. If we delay to the schema changer then we can't do
BEGIN;
DROP TABLE t;
DROP TYPE <t used to use>
END;
I'm not sure that there is a problem with doing it in the drop table txn.

I think I'm confused. Can't we just make sure that one of the two operations cleans up the back reference?

rohany · 2020-07-21T15:00:23Z

I think I'm confused. Can't we just make sure that one of the two operations cleans up the back reference?

Yeah we can, I just wasn't sure in which place we should do it. We had a small discussion about moving it after the drop txn commits to maybe avoid that publish multiple stale state issue, but this wasn't the problem.

rohany · 2020-07-21T15:40:59Z

Updated to do the backreference removal in the DROP TABLE txn.

rohany · 2020-07-21T17:58:26Z

cc @knz for some comments you had:

pkg/sql/schema_changer.go, line 468 at r3 (raw file):

	// If this table is being dropped, remove all back references from
	// types that this table uses. First collect all types referenced
	// by the table.

I don't understand why this is happening only here. Why are the back references not removed as soon as the DROP statement executes, like we already drop the back references for views etc?

Lucy should help here.

-- This has been updated here.

pkg/sql/schema_changer.go, line 1033 at r3 (raw file):

	// Now that all mutations have been applied, find the new set of referenced
	// type descriptors. If this table has been dropped in the mean time, then

ditto

-- This case is a bit different. When dropping a particular schema element, we can only remove the back reference once the state machine through the schema changer finishes. Imagine dropping a column with a user defined type -- the column is still present on the table while it is in the DELETE_ONLY and DELETE_AND_WRITE_ONLY states.

pkg/sql/sqlbase/structured.go, line 1358 at r3 (raw file):

}
// Sort the output so that the order is deterministic.
sort.Sort(result)

What's going on here? If you think this is necessary, it sounds to me as if it would have been necessary earlier (i.e. it's a bug fix for an outstanding issue). In that case I would request that you extract this into a separate commit, add the tests and refer to the issue that it closes.

-- This sorting is only became necessary here so that we add back references in the same order every time. This just ensures that the output list of dependent tables is always the same.

pkg/sql/sqlbase/type_desc.go, line 319 at r3 (raw file):

// AddReferencingDescriptorID adds a new referencing descriptor ID to the
// TypeDescriptor. It ensures that duplicates are not added.
This machinery should be shared for all inter-object references, not just UDTs.

-- I agree, but it's somewhat more difficult to do for tables than for types. For tables the backreference has to be inserted into the correct child object depending on what kind of reference is being made. For types we don't do such a thing.

rohany · 2020-07-27T14:11:51Z

Friendly ping!

thoszhang

Reviewed 1 of 17 files at r1, 19 of 19 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @RichardJCai, and @rohany)

pkg/sql/drop_type.go, line 69 at r3 (raw file):

		// Get the array type that needs to be dropped as well.
		// TODO (rohany): This should be a method on the desc.Collection.

What's currently preventing this (here and elsewhere)?

pkg/sql/catalog/lease/lease.go, line 414 at r3 (raw file):

				}
				if descsToUpdate[id] == nil {
					return sqlbase.ErrDescriptorNotFound

Maybe add a comment with the PublishMultiple issue here?

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @RichardJCai, and @rohany)

pkg/sql/drop_type.go, line 69 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

What's currently preventing this (here and elsewhere)?

Nothing, this PR was just out of date. Updated.

pkg/sql/catalog/lease/lease.go, line 414 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Maybe add a comment with the PublishMultiple issue here?

Done.

rohany · 2020-07-27T20:25:03Z

I'll let @ajwerner take a final pass too

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @RichardJCai, and @rohany)

pkg/sql/sqlbase/structured.go, line 3916 at r4 (raw file):

		return t.Table.Dropped()
	case *Descriptor_Type:
		return t.Type.Dropped()

Good catch

This PR implements the `DROP TYPE` command without support for `CASCADE`. It lays the foundation of the dependency management between types and tables that use them. With this, we can drop types when a database is dropped. Release note: None

ajwerner

We'll need to be careful in DROP DATABASE to do the right thing when dropping referenced types.

Reviewed 2 of 11 files at r4, 1 of 2 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @RichardJCai, and @rohany)

rohany · 2020-07-28T14:30:57Z

We'll need to be careful in DROP DATABASE to do the right thing when dropping referenced types.

see #51362 :)

rohany · 2020-07-28T15:12:15Z

bors r+

craig · 2020-07-28T16:14:52Z

Build succeeded:

GitHub CI (Cockroach)

rohany force-pushed the drop-type branch from 7298777 to 8733789 Compare July 9, 2020 20:44

rohany force-pushed the drop-type branch 4 times, most recently from cdeed48 to 8a8460c Compare July 15, 2020 20:29

rohany marked this pull request as ready for review July 15, 2020 20:29

rohany requested review from ajwerner and thoszhang July 15, 2020 20:29

RichardJCai suggested changes Jul 20, 2020

View reviewed changes

pkg/sql/drop_type.go Show resolved Hide resolved

rohany force-pushed the drop-type branch from 8a8460c to a8a848b Compare July 20, 2020 21:44

ajwerner suggested changes Jul 21, 2020

View reviewed changes

rohany force-pushed the drop-type branch from a8a848b to f366923 Compare July 21, 2020 14:23

rohany commented Jul 21, 2020

View reviewed changes

ajwerner reviewed Jul 21, 2020

View reviewed changes

rohany force-pushed the drop-type branch 2 times, most recently from d085da5 to f495038 Compare July 21, 2020 15:39

RichardJCai mentioned this pull request Jul 21, 2020

sql: adding privileges to types #51622

Merged

rohany force-pushed the drop-type branch from f495038 to 7bc4234 Compare July 22, 2020 16:49

rohany mentioned this pull request Jul 22, 2020

backupccl: restores referencing existing types need to track backreferences #51787

Closed

rohany requested a review from ajwerner July 23, 2020 21:12

thoszhang approved these changes Jul 27, 2020

View reviewed changes

rohany force-pushed the drop-type branch from 7bc4234 to e1dbabf Compare July 27, 2020 20:22

rohany commented Jul 27, 2020

View reviewed changes

thoszhang reviewed Jul 27, 2020

View reviewed changes

rohany requested a review from RichardJCai July 27, 2020 20:58

rohany force-pushed the drop-type branch from e1dbabf to 65cb75e Compare July 28, 2020 01:25

sql: implement the DROP TYPE command

2dbcc20

This PR implements the `DROP TYPE` command without support for `CASCADE`. It lays the foundation of the dependency management between types and tables that use them. With this, we can drop types when a database is dropped. Release note: None

rohany force-pushed the drop-type branch from 65cb75e to 2dbcc20 Compare July 28, 2020 14:27

ajwerner approved these changes Jul 28, 2020

View reviewed changes

craig bot merged commit c8559fe into cockroachdb:master Jul 28, 2020

Conversation

rohany commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jul 9, 2020

Uh oh!

rohany commented Jul 9, 2020

Uh oh!

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

rohany left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

rohany commented Jul 21, 2020

Uh oh!

rohany commented Jul 21, 2020

Uh oh!

rohany commented Jul 21, 2020

Uh oh!

rohany commented Jul 27, 2020

Uh oh!

thoszhang left a comment

Choose a reason for hiding this comment

Uh oh!

rohany left a comment

Choose a reason for hiding this comment

Uh oh!

rohany commented Jul 27, 2020

Uh oh!

thoszhang left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

rohany commented Jul 28, 2020

Uh oh!

rohany commented Jul 28, 2020

Uh oh!

craig bot commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rohany commented Jul 9, 2020 •

edited

Loading