sql,externalconn: add owner_id column to system.external_connections

[andyyang890]

This patch adds a new owner_id column to the system.external_connections
table, which corresponds to the existing owner column, in order to bring us
closer to the eventual goal of allowing renaming of users. Migrations are
also added to alter and backfill the table in older clusters.

Part of #87079
Part of #92342

Release note: None

[cockroach-teamcity]

This change is 

[stevendanna]

Thanks! It might be good to put why we are adding this field in the PR/commit message somewhere.

We also need to handle external_connections tables in backups that don't the owner_id column. Check out 1bc6c6d for an example.

[andyyang890]

It might be good to put why we are adding this field in the PR/commit message somewhere.

Done.

We also need to handle external_connections tables in backups that don't the owner_id column.

Yep, I'm planning on doing that in a followup PR where I set the column to be NOT NULL.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andyyang890, @dt, and @stevendanna)

---

pkg/cloud/externalconn/record.go line 355 at r2 (raw file):

		case `owner`:
			arg = tree.NewDString(e.rec.Owner.Normalized())
		case `owner_id`:

it looks like this change will cause the owner_id column to always be stored. but we need to gate it on the clusterversion you just added.

and it would be good to have a test that shows it working fine in a mixed version cluster. let's add another mixed-version logic test using the new framework (which is skipped right now, but you should be able to run locally)

---

pkg/sql/create_external_connection.go line 141 at r2 (raw file):

	row, err := txn.QueryRowEx(params.ctx, `get-user-id`, txn.KV(),
		sessiondata.NodeUserSessionDataOverride,
		`SELECT user_id FROM system.users WHERE username = $1`,

in the future, we should add another function similar to p.User() that returns the user ID instead. that way we can cache the user ID in one place, and avoid having to do this lookup query in many places. could you file a github issue for that, and make a comment here that references it?

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, @rafiss, and @stevendanna)

---

pkg/sql/create_external_connection.go line 141 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

in the future, we should add another function similar to p.User() that returns the user ID instead. that way we can cache the user ID in one place, and avoid having to do this lookup query in many places. could you file a github issue for that, and make a comment here that references it?

Done. See #98170

[andyyang890]

FYI I rebased this onto #98163 so the first commit is from that.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, @rafiss, and @stevendanna)

---

pkg/cloud/externalconn/record.go line 355 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it looks like this change will cause the owner_id column to always be stored. but we need to gate it on the clusterversion you just added.

and it would be good to have a test that shows it working fine in a mixed version cluster. let's add another mixed-version logic test using the new framework (which is skipped right now, but you should be able to run locally)

I added some gating by threading through a map of excluded columns from the relevant planner function since I don't think this function has access to the version handle, but I'm not sure if this is the best approach. Let me know if you have any other ideas!

---

pkg/upgrade/upgrades/external_connections_table_user_id_migration_test.go line 20 at r4 (raw file):

	"github.com/cockroachdb/cockroach/pkg/base"
	_ "github.com/cockroachdb/cockroach/pkg/cloud/userfile"

I had to add this import in order to trigger the init() function to make the CREATE EXTERNAL CONNECTION statement below work.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andyyang890, @dt, and @stevendanna)

---

pkg/cloud/externalconn/record.go line 355 at r2 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

I added some gating by threading through a map of excluded columns from the relevant planner function since I don't think this function has access to the version handle, but I'm not sure if this is the best approach. Let me know if you have any other ideas!

this looks like it works, but i'd be curious if @stevendanna or @adityamaru have opinions on how to version gate this, since they own this area of the code.

---

pkg/sql/logictest/testdata/logic_test/testserver_22.2_23.1_external_connections_user_id_upgrade line 3 at r5 (raw file):

# LogicTest: cockroach-go-testserver-22.2-master

# Verify that all nodes are running 22.2 binaries.

nit: i think we can remove these checks for 22.2 binaries. they were there in the first mixed version test more as a sanity check / proof-of-concept

---

pkg/sql/logictest/testdata/logic_test/testserver_22.2_23.1_external_connections_user_id_upgrade line 52 at r5 (raw file):

upgrade 2

# Verify that all nodes are now running 23.1 binaries.

ditto above

[adityamaru]

Thanks for doing this!

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, @rafiss, and @stevendanna)

---

pkg/sql/logictest/testdata/logic_test/testserver_22.2_23.1_external_connections_user_id_upgrade line 3 at r5 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: i think we can remove these checks for 22.2 binaries. they were there in the first mixed version test more as a sanity check / proof-of-concept

Done.

---

pkg/sql/logictest/testdata/logic_test/testserver_22.2_23.1_external_connections_user_id_upgrade line 52 at r5 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ditto above

Done.

[andyyang890]

TFTRs!

bors r=rafiss,adityamaru

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[andyyang890]

bors r=rafiss,adityamaru

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[andyyang890]

bors r=rafiss,adityamaru

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)