sql: Support CREATE DATABASE WITH OWNER

[Fenil-P]

fixes #67817

Release note (sql change): Allow users to specify the owner when creating a database.
Similar to postgresql: CREATE DATABASE name [ [ WITH ] [ OWNER [=] user_name ]

[cockroach-teamcity]

This change is 

[RichardJCai]

Nice work, looks mostly good to me after minor comments are addressed.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Fenil-P and @RichardJCai)

---

pkg/sql/descriptor.go, line 147 at r1 (raw file):

	// database-level zone configuration if there is a region config on the
	// descriptor.
	//p.setNewDatabaseOwner(ctx)

Remove this comment.

---

pkg/sql/descriptor.go, line 148 at r1 (raw file):

	// descriptor.
	//p.setNewDatabaseOwner(ctx)
	if database.Owner.Name != "" {

Use Owner.IsUndefined()

---

pkg/sql/descriptor.go, line 154 at r1 (raw file):

		}

		if err := p.checkCanAlterToNewOwner(ctx, desc, newOwner); err != nil {

Not 100% sure if we need this check if we're creating a new database here.
Can you double check with Postgres - you'll have to download PG and verify.

Namely, check if you can CREATE DATABASE ... WITH OWNER ... where the current user is not a member of the owner role.

Also please add a test for this if it turns out we do need this check (verify that we need to be a member of the owner role)

---

pkg/sql/descriptor.go, line 161 at r1 (raw file):

			return nil, true, err
		}
		if err := p.writeNonDropDatabaseChange(

Don't need this since the database descriptor gets written later in this function.

[Fenil-P]

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

---

pkg/sql/descriptor.go, line 147 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Remove this comment.

Done.

---

pkg/sql/descriptor.go, line 148 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Use Owner.IsUndefined()

Done.

---

pkg/sql/descriptor.go, line 154 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Not 100% sure if we need this check if we're creating a new database here.
Can you double check with Postgres - you'll have to download PG and verify.

Namely, check if you can CREATE DATABASE ... WITH OWNER ... where the current user is not a member of the owner role.

Also please add a test for this if it turns out we do need this check (verify that we need to be a member of the owner role)

Done.

---

pkg/sql/descriptor.go, line 161 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Don't need this since the database descriptor gets written later in this function.

Done.

[RichardJCai]

Great work! LGTM after addressing nit

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Fenil-P and @RichardJCai)

---

pkg/sql/descriptor.go, line 140 at r2 (raw file):

	} else {
		owner = p.SessionData().User()

nit: we can just remove this else block and initially set owner to p.SessionData.User() and update it if the if cond is met

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Fenil-P, @rafiss, and @RichardJCai)

---

pkg/sql/descriptor.go, line 140 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: we can just remove this else block and initially set owner to p.SessionData.User() and update it if the if cond is met

+1

actually when is database.Owner ever undefined?

---

pkg/sql/parser/sql.y, line 8923 at r2 (raw file):

  OWNER opt_equal role_spec
  {
    $$ = $3

should this be $$.val = $3.roleSpec()

maybe it's the same thing?

---

pkg/sql/parser/sql.y, line 8928 at r2 (raw file):

   {
		 $$.val = tree.RoleSpec{
		   RoleSpecType: tree.SessionUser,

why SessionUser and not CurrentUser?

[Fenil-P]

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

---

pkg/sql/descriptor.go, line 140 at r2 (raw file):

actually when is database.Owner ever undefined?
They're undefined when defaultdb and postgres are being created

---

pkg/sql/parser/sql.y, line 8923 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should this be $$.val = $3.roleSpec()

maybe it's the same thing?

I kept the opt_owner_clause the same type as role_spec so its done later on in the create db statement.

---

pkg/sql/parser/sql.y, line 8928 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

why SessionUser and not CurrentUser?

Updated

[rafiss]

nice!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Fenil-P, @rafiss, and @RichardJCai)

---

pkg/sql/descriptor.go, line 140 at r2 (raw file):

Previously, Fenil-P (Fenil Patel) wrote…

actually when is database.Owner ever undefined?
They're undefined when defaultdb and postgres are being created

that happens in createDefaultDbs in startupmigrations with CREATE DATABASE IF NOT EXISTS "%s"

but based on how you added the new syntax, shouldn't an empty owner clause already default to using the current user?

anyway, this doesn't need to block the PR, i was just curious. having the undefined fallback case seems good regardless

[Fenil-P]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[Fenil-P]

bors retry

[craig]

Already running a review

[Fenil-P]

bors r+

[craig]

Already running a review

[craig]

Build succeeded:

• GitHub CI (Cockroach)