sql: handle companion array name conflicts in ALTER TYPE

[bghal]

In cases of a name collisions of a companion array type when changing a
type's schema, the schemachanger was not handling it well. This change
addresses that and adds a test.

Supports: #168255

Part of: #164216

Epic: CRDB-31325

Release note: None

[trunk-io]

😎 Merged successfully - details.

[cockroach-teamcity]

This change is 

[rafiss]

@rafiss made 3 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on bghal).

---

pkg/sql/alter_type.go line 375 at r1 (raw file):

	}

	if err := descs.CheckObjectNameCollision(

suggestion: Add a short comment explaining why a second collision check
exists. prepareSetSchema (called above) already checks the primary type's
name, but it has no awareness of the companion array type. a future reader
might benefit from a comment like:

  // prepareSetSchema only checks the primary type's name. The companion
  // array type is renamed in lockstep below, so verify its name does not
  // collide in the destination schema before we mutate anything.

perhaps also explaining how this differs from renameType
(which auto-resolves array conflicts via findFreeArrayTypeName) would help
future readers. The asymmetry matches PostgreSQL: ALTER TYPE ... RENAME
auto-renames the companion array, while ALTER TYPE ... SET SCHEMA errors,
but that's not obvious from the code alone.

---

pkg/sql/logictest/testdata/logic_test/set_schema line 542 at r1 (raw file):

# The companion array type "_salmon" collides.
statement error pq: type "test.stream._salmon" already exists
ALTER TYPE ocean.salmon SET SCHEMA stream

suggestion: Consider adding an explicit assertion that the target schema
was untouched, e.g.

  statement error pq: type "stream.salmon" does not exist
  SELECT NULL::stream.salmon

The current test confirms the source type/array still work, but never
verifies the failure aborted before any move. Since the bug being fixed
is precisely the partial-state window between the two
performRenameTypeDesc calls, an explicit "target schema is empty"
assertion would lock that down.

---

pkg/sql/logictest/testdata/logic_test/set_schema line 549 at r1 (raw file):

statement ok
SELECT ARRAY['sockeye']::ocean._salmon

suggestion: Consider a second case where the conflicting _salmon is a
TABLE (or sequence) rather than a type, since descs.CheckObjectNameCollision
matches across descriptor kinds. PostgreSQL also errors in that case (with
the same 42710). I think CRDB already would too, but this is worth testing officially.

[blathers-crl]

Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link)

[bghal]

@bghal made 3 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on rafiss).

---

pkg/sql/alter_type.go line 375 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

suggestion: Add a short comment explaining why a second collision check
exists. prepareSetSchema (called above) already checks the primary type's
name, but it has no awareness of the companion array type. a future reader
might benefit from a comment like:

  // prepareSetSchema only checks the primary type's name. The companion
  // array type is renamed in lockstep below, so verify its name does not
  // collide in the destination schema before we mutate anything.

perhaps also explaining how this differs from renameType
(which auto-resolves array conflicts via findFreeArrayTypeName) would help
future readers. The asymmetry matches PostgreSQL: ALTER TYPE ... RENAME
auto-renames the companion array, while ALTER TYPE ... SET SCHEMA errors,
but that's not obvious from the code alone.

Done.

---

pkg/sql/logictest/testdata/logic_test/set_schema line 542 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

suggestion: Consider adding an explicit assertion that the target schema
was untouched, e.g.

  statement error pq: type "stream.salmon" does not exist
  SELECT NULL::stream.salmon

The current test confirms the source type/array still work, but never
verifies the failure aborted before any move. Since the bug being fixed
is precisely the partial-state window between the two
performRenameTypeDesc calls, an explicit "target schema is empty"
assertion would lock that down.

Done.

---

pkg/sql/logictest/testdata/logic_test/set_schema line 549 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

suggestion: Consider a second case where the conflicting _salmon is a
TABLE (or sequence) rather than a type, since descs.CheckObjectNameCollision
matches across descriptor kinds. PostgreSQL also errors in that case (with
the same 42710). I think CRDB already would too, but this is worth testing officially.

Done.

[blathers-crl]

Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link)

[rafiss]

lgtm! nice find

/trunk merge

@rafiss made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained.

[blathers-crl]

Detected infrastructure failure on trunk-merge branch (matched: self-hosted runner lost communication with the server). Automatically resubmitting to merge queue (attempt 1 of 2). (run link)

[blathers-crl]

/trunk merge