sql: block concurrent renames with declarative schema changes

[fqazi]

Previously, we allowed the legacy and declarative schema change jobs to run concurrently with declarative schema change jobs, for example renames of objects. If a drop operation was executed next to a legacy schema change job, it was possible for the legacy code to delete the descriptor before the declarative schema changer. To address this, we are going to safe guard rename operations, which currently do not check for declarative schema changer jobs.

Fixes: #128268

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

nice find!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/sql/rename_table.go line 121 at r1 (raw file):

	// Exit early with an error if the table is undergoing a declarative schema
	// change.
	if catalog.HasConcurrentDeclarativeSchemaChange(n.tableDesc) {

ah interesting. i thought we already automatically blocked all legacy schema changes if HasConcurrentDeclarativeSchemaChange is true.

is there a way to make that happen, rather than needing to remember to add this in each legacy schema change op?

---

pkg/sql/schemachanger/schemachanger_test.go line 630 at r1 (raw file):

		decl := rand.Intn(2) == 0
		if !decl {
			_, err := sqlDB.Exec("SET use_declarative_schema_changer='off';")

technically sqlDB is a connection pool, so if we want this to be reliable, we would need to change the test to operate at the connection level. or maybe we can just set sqlDB.SetMaxOpenConns(1) at the beginning of the test.

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/schemachanger/schemachanger_test.go line 630 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

technically sqlDB is a connection pool, so if we want this to be reliable, we would need to change the test to operate at the connection level. or maybe we can just set sqlDB.SetMaxOpenConns(1) at the beginning of the test.

Done.

I went with a middle ground approach where I opened one pool per worker with a connection limit.

---

pkg/sql/rename_table.go line 121 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah interesting. i thought we already automatically blocked all legacy schema changes if HasConcurrentDeclarativeSchemaChange is true.

is there a way to make that happen, rather than needing to remember to add this in each legacy schema change op?

Done.

I pushed this into the lower layers of when we create the jobs, which should impact everything, except for GRANT statements.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/sql/table.go line 278 at r2 (raw file):

			tableDesc.Name)
	}
	// Exit early with an error if the table is undergoing a declarative schema

would we need the same check for schema changes on types or functions?

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/table.go line 278 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

would we need the same check for schema changes on types or functions?

Good point for types this PR already has coverage via writeTypeSchemaChange. For functions only dropping involves jobs and thats covered by dropFunctionImpl.

[fqazi]

@rafiss TFTR!

bors r+

[craig]

Build succeeded:

• check_generated_code
• macos_arm64_build
• docker_image_amd64
• unit_tests
• linux_amd64_fips_build
• windows_build
• examples_orms
• local_roachtest
• local_roachtest_fips
• lint
• linux_amd64_build
• linux_arm64_build
• acceptance
• macos_amd64_build

[fqazi]

blathers backport 23.2

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from ae0b1f8 to blathers/backport-release-23.2-128683: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2 failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[fqazi]

blathers backport 24.1

[blathers-crl]

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

---

Issue #128268: branch-release-24.1.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[fqazi]

blathers backport 24.2

[blathers-crl]

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

---

Issue #128268: branch-release-24.2.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}