sql: allow atomic name swaps

[postamar]

This PR deprecates and disables usage of the draining names mechanism on catalog descriptors. When renaming or dropping a table/database/schema/type, the namespace entry is now modified in-transaction. This makes it possible to swap names in-transaction. On the other hand, names may be inconsistently resolved to either the old or the new target in specific circumstances.

Fixes #54562.

[cockroach-teamcity]

This change is 

[postamar]

This PR is best reviewed commit-by-commit. We might want more tests but I'm not quite sure what to add. I'm all ears.

[ajwerner]

I'd very much like to see logic tests that exercise these atomic name changes in various ways. I don't see any new logic tests.

---

Some more snippets that I think should get some updated commentary or, at least, consideration:

cockroach/pkg/sql/catalog/descs/kv_descriptors.go

Lines 192 to 197 in 9dde19c

	if desc.GetName() != name {
	// Immediately after a RENAME an old name still points to the descriptor
	// during the drain phase for the name. Do not return a descriptor during
	// draining.
	return nil, nil
	}

cockroach/pkg/sql/catalog/descs/uncommitted_descriptors.go

Lines 94 to 100 in 9dde19c

	// descNames is the set of names which a read or written
	// descriptor took on at some point in its lifetime. Everything added to
	// uncommittedDescriptors is added to descNames as well
	// as all of the known draining names. The idea is that if we find that
	// a name is not in the above map but is in the set, then we can avoid
	// doing a lookup.
	descNames nstree.Set

After very brief reflection, I think the second piece could still be relevant.

Reviewed 6 of 25 files at r1, 11 of 15 files at r2, 33 of 37 files at r3, 12 of 12 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

[postamar]

Thanks for taking a look!

I've added a new logic test suite to exercise these changes. Did I cover enough cases? I'm honestly bad at coming up with these.

I've added TODOs for each of these snippets. Mixed-version clusters mean that we can't do what we really want to do in the descs collection until 22.1 comes out anyway.

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

[ajwerner]

I've added TODOs for each of these snippets. Mixed-version clusters mean that we can't do what we really want to do in the descs collection until 22.1 comes out anyway.

Ah yeah, makes sense. I'm a bit sad I wasn't in the mixed-version mindset in the first read through.

---

abstract thought: are there any hazards with drop cascades and then failures to resolve fully qualified names?

---

On the whole, . Let me stew on some concerning thoughts until tomorrow but I'm pretty pleased with this.

Reviewed 1 of 52 files at r5, 4 of 37 files at r6, 9 of 15 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/migration/migrations/ensure_no_draining_names.go, line 44 at r7 (raw file):

	cte2
WHERE
	COALESCE(json_array_length(d->'drainingNames'), 0) > 0;

add a LIMIT 1 then consider just using the Buffered version of the internal executor for some simpler processing?

---

pkg/sql/logictest/testdata/logic_test/rename_atomic, line 4 at r7 (raw file):

CREATE DATABASE db;
CREATE DATABASE db_new;

what I'd like to see added here is the usage of the renamed objects in the transaction after the renaming statements to demonstrate that they are indeed usable.

---

pkg/sql/logictest/testdata/logic_test/table, line 859 at r7 (raw file):

user root

it feels like you could preserve a version of this test without the errors

---

pkg/sql/logictest/testdata/logic_test/views, line 848 at r7 (raw file):

CREATE VIEW db2.v2 AS SELECT a+b+c+d FROM cd, db1.public.ab

# Unit test for #60737

it feels like you can rework this test to be a working test if you remove the errors and the rollbacks.

[postamar]

abstract thought: are there any hazards with drop cascades and then failures to resolve fully qualified names?

I honestly couldn't tell you at this point, could you elaborate please? It's been a while. Do you have in mind the following: adding a unit test which drop-cascades a bunch of stuff, blocks the schema change job via a testing knob, and concurrently tries to resolve fully-qualified names? Basically testing that the job does the "right thing" with names now that it no longer drains the names at the very end of its execution?

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

---

pkg/migration/migrations/ensure_no_draining_names.go, line 44 at r7 (raw file):

Previously, ajwerner wrote…

add a LIMIT 1 then consider just using the Buffered version of the internal executor for some simpler processing?

Done.

---

pkg/sql/logictest/testdata/logic_test/rename_atomic, line 4 at r7 (raw file):

Previously, ajwerner wrote…

what I'd like to see added here is the usage of the renamed objects in the transaction after the renaming statements to demonstrate that they are indeed usable.

Done, I think.

---

pkg/sql/logictest/testdata/logic_test/table, line 859 at r7 (raw file):

Previously, ajwerner wrote…

it feels like you could preserve a version of this test without the errors

That's true, but what are we testing in that case? It feels like the purpose of this deleted test is to exercise the error messages. Would you like me to extend the rename_atomic suite with cases involving different object types: tables, types, views etc?

---

pkg/sql/logictest/testdata/logic_test/views, line 848 at r7 (raw file):

Previously, ajwerner wrote…

it feels like you can rework this test to be a working test if you remove the errors and the rollbacks.

Same comment as above regarding what this test exercises.

[ajwerner]

LGTM

[postamar]

Thanks for the review!

[ajwerner]

mod the one comment

Reviewed 2 of 53 files at r9, 6 of 37 files at r10, 9 of 15 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/migration/migrations/ensure_no_draining_names.go, line 75 at r11 (raw file):

		log.Infof(ctx, "descriptor with ID %d and name %q still has draining names", id, name)
	}
	panic("unreachable")

how is this unreachable? If the context gets canceled, won't you be here?

---

pkg/sql/logictest/testdata/logic_test/table, line 859 at r7 (raw file):

Previously, postamar (Marius Posta) wrote…

That's true, but what are we testing in that case? It feels like the purpose of this deleted test is to exercise the error messages. Would you like me to extend the rename_atomic suite with cases involving different object types: tables, types, views etc?

It's okay, I think it's fine.

[postamar]

how is this unreachable? If the context gets canceled, won't you be here?

Gaah I keep forgetting about that. Thanks, good catch! Returning ctx.Err() instead, on your advice (thank you for that too).

[postamar]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)