sql: disallow table/view/sequence rename from making cross DB references

[fqazi]

Fixes: #55709

Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.

Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 1 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

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

		}
	}

nit: this is a pretty big chunk of new logic. Any interest in reworking this into some functions? It'll also allow for some early returns which I suspect will aid readability.

One thing I lament is how hard it is to write unit tests for this stuff. The reason is pretty clear: constructing descriptors is awful. My sincere dream is that we'll get to a point where we can write SQL strings to build descriptors in memory and then unit test stuff like this. The fact that we go all the way to the logic tests pretty much always makes me

[ajwerner]

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

---

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

Previously, ajwerner wrote…

nit: this is a pretty big chunk of new logic. Any interest in reworking this into some functions? It'll also allow for some early returns which I suspect will aid readability.

One thing I lament is how hard it is to write unit tests for this stuff. The reason is pretty clear: constructing descriptors is awful. My sincere dream is that we'll get to a point where we can write SQL strings to build descriptors in memory and then unit test stuff like this. The fact that we go all the way to the logic tests pretty much always makes me

sad, I was missing the word sad.

[fqazi]

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

---

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

Previously, ajwerner wrote…

sad, I was missing the word sad.

Let me rework this into different functions, it would aid with readability.

[fqazi]

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

---

pkg/sql/logictest/testdata/logic_test/rename_table, line 466 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

This comment applies to all the lines added above: can you please conform the style of your SQL statements & queries to the existing standard? The existing standard is defined by https://sqlfum.pt/ which I'm told was built using the CRDB SQL parser.

Done.

[ajwerner]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @postamar)

---

pkg/sql/rename_table.go, line 325 at r2 (raw file):

					Required: true,

I think this should skip the cache. Generally in the context of schema changes we want a consistent view of descriptors.

---

pkg/sql/rename_table.go, line 429 at r2 (raw file):

			allDescriptors, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)

This feels egregiously expensive. I'm pretty certain we track the backreference on the columns. See descpb.ColumnDescriptor.OwnsSequenceIDs

[fqazi]

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

---

pkg/sql/rename_table.go, line 325 at r2 (raw file):

Previously, ajwerner wrote…

					Required: true,

I think this should skip the cache. Generally in the context of schema changes we want a consistent view of descriptors.

Done.

---

pkg/sql/rename_table.go, line 429 at r2 (raw file):

Previously, ajwerner wrote…

			allDescriptors, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)

This feels egregiously expensive. I'm pretty certain we track the backreference on the columns. See descpb.ColumnDescriptor.OwnsSequenceIDs

Done

[ajwerner]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

[fqazi]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)