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: add support for dropping the primary region from a multi-region db #60437

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

arulajmani
Copy link
Collaborator

This patch adds support for dropping the primary region from a
multi-region database. Dropping the primary region is allowed only if
all other regions have been removed from the database. Put another way,
the primary region is always the last region to go and the last region
to go is always the primary region.

As part of removing the last region from the database, all of the
static sql state is also reverted, leaving no detritus from
multi-region ways of old. In particular, this means that all GLOBAL
and REGIONAL BY TABLE tables lose the locality config from their
descriptor and the multi-region type descriptor is removed from the
database.

Dropping the last region can fail for a few of reasons:

  • A REGIONAL BY TABLE table is homed explicitly in the region being
    dropped.
  • A REGIONAL BY ROW table exists in the database (which must write the
    last region in all rows of its region column).
  • A table in the database uses the multi-region enum.

All of these fail scenarios can be ascertained by checking if
references on the type descriptor exist, without having to go through
the general case of enum value removal that happens in the type schema
changer. This is what we do.

Lastly, it's worth noting this patch doesn't modify zone configurations.

Closes #57391
Informs #58333

Release note (sql change): ALTER DATABASE ... DROP REGION now allows
for dropping the primary region.

@arulajmani arulajmani requested review from otan, ajstorm and a team February 10, 2021 19:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@arulajmani arulajmani force-pushed the mr-drop-primary-region branch from 232037b to 69a6f26 Compare February 10, 2021 19:40
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @arulajmani)


pkg/sql/alter_database.go, line 308 at r1 (raw file):

				errors.Newf("cannot drop region %q", dbDesc.RegionConfig.PrimaryRegion),
				"You must designate another region as the primary region or remove all "+
					"other regions before attempting to drop region %q", n.Region,

can you give the syntax to do so?


pkg/sql/alter_database.go, line 386 at r1 (raw file):

		for _, desc := range n.toDrop {
			jobDesc := fmt.Sprintf("drop multi-region enum with ID %d", desc.ID)
			err := params.p.dropTypeImpl(params.ctx, desc, jobDesc, true /* queueJob */)

is dropping a type async? what happens if the type drop fails but we've already removed all the locality conifgs?


pkg/sql/logictest/testdata/logic_test/multiregion, line 989 at r1 (raw file):


# Cannot drop the primary region yet, as there are other regions in the db.
statement error pq: cannot drop region "us-east-1"

is there a detail that this is the primary region which is why it cannot be dropped

This patch adds support for dropping the primary region from a
multi-region database. Dropping the primary region is allowed only if
all other regions have been removed from the database. Put another way,
the primary region is always the last region to go and the last region
to go is always the primary region.

As part of removing the last region from the database, all of the
static sql state is also reverted, leaving no detritus from
multi-region ways of old. In particular, this means that all GLOBAL
and REGIONAL BY TABLE tables lose the locality config from their
descriptor and the multi-region type descriptor is removed from the
database.

Dropping the last region can fail for a few of reasons:
- A REGIONAL BY TABLE table is homed explicitly in the region being
dropped.
- A REGIONAL BY ROW table exists in the database (which must write the
last region in all rows of its region column).
- A table in the database uses the multi-region enum.

All of these fail scenarios can be ascertained by checking if
references on the type descriptor exist, without having to go through
the general case of enum value removal that happens in the type schema
changer. This is what we do.

Lastly, it's worth noting this patch doesn't modify zone configurations.

Closes cockroachdb#57391
Informs cockroachdb#58333

Release note (sql change): `ALTER DATABASE ... DROP REGION` now allows
for dropping the primary region.
@arulajmani arulajmani force-pushed the mr-drop-primary-region branch from 69a6f26 to 14af57d Compare February 11, 2021 01:17
@blathers-crl blathers-crl bot requested a review from otan February 11, 2021 01:17
Copy link
Collaborator Author

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use another squiz @otan

Did I do it right?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)


pkg/sql/alter_database.go, line 308 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

can you give the syntax to do so?

Done.


pkg/sql/alter_database.go, line 386 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

is dropping a type async? what happens if the type drop fails but we've already removed all the locality conifgs?

We talked about this offline, but typing this out to capture that conversation. I think it should be fine because dropping a type constitutes of writing the type descriptor in DROPPED state. The async part is the GC and the namespace table name reclaim. Even if the async job fails or gets cancelled, it is no different to how this would work today for regular type descriptors -- an attempt would be made to reclaim the namespace entries again until successful (using the jobs infra) and I think the dropped descriptor will just go away when gc-ttl comes around. I think we should be fine.


pkg/sql/logictest/testdata/logic_test/multiregion, line 989 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

is there a detail that this is the primary region which is why it cannot be dropped

One of those cases which comes with a hint but I didn't have one before my rebase and didn't remember to add one after. Fixed with the hint now.

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @otan)


pkg/sql/alter_database.go, line 386 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

We talked about this offline, but typing this out to capture that conversation. I think it should be fine because dropping a type constitutes of writing the type descriptor in DROPPED state. The async part is the GC and the namespace table name reclaim. Even if the async job fails or gets cancelled, it is no different to how this would work today for regular type descriptors -- an attempt would be made to reclaim the namespace entries again until successful (using the jobs infra) and I think the dropped descriptor will just go away when gc-ttl comes around. I think we should be fine.

a test would be great to ensure this doesn't regress, but happy for this to be a separate PR.

@arulajmani arulajmani requested a review from otan February 11, 2021 02:00
@arulajmani
Copy link
Collaborator Author

a test would be great to ensure this doesn't regress, but happy for this to be a separate PR.

I agree. Half way into writing the test, I realized this will need a new testing knob, so now Im inclined to do it in a separate commit.

bors r=otan

@craig
Copy link
Contributor

craig bot commented Feb 11, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: allow removing the last region from a database
3 participants