builtins: add create_tenant(<tenant-name>) overload by adityamaru · Pull Request #91518 · cockroachdb/cockroach

adityamaru · 2022-11-08T16:27:23Z

This change adds another overload to the crdb_internal.create_tenant builtin that only accepts the name the of the tenant to be created. The underlying logic automatically picks the next unique ID based on the existing tenants in the system.tenants table. The builtin returns the ID assigned to the newly created tenant or an error if a tenant with the same name already exists. Just like other overloads this buitlin is admin-only and can only be run by the system tenant.

Informs: #91235

Release note: None

cockroach-teamcity · 2022-11-08T16:27:31Z

This change is

pkg/sql/tenant.go

lidorcarmel

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @knz)

pkg/sql/tenant.go line 236 at r1 (raw file):

func (p *planner) CreateTenant(ctx context.Context, name string) (roachpb.TenantID, error) {
	if err := p.RequireAdminRole(ctx, "create tenant"); err != nil {
		return roachpb.SystemTenantID, err

why a real id on failure and not roachpb.TenantID{} as you do below?

Code quote:

roachpb.SystemTenantID

pkg/sql/tenant.go line 240 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

@knz just confirming this query could result in us re-using a previously used ID, but this is okay because the tenant has to be GC'ed for us to be able to reuse the ID.

Alternatively, we could change this query to just find the higher PK id and just use the next one.

I think changing this as you say makes sense. otherwise when debugging we will always need to think about "is id x the same x as the one we deleted yesterday?" and stuff like that. even if it's rare, having an id that was never used before simplifies things.

thinking a bit.. IIUC we allow creating tenants with whatever id the caller wants, right? so if I create a tenant with id MAX-1, that's not going to be great if we change this code to use the max id we ever saw plus one, right? in that case it's not going to work and the existing code is better.

pkg/sql/tenant.go line 244 at r1 (raw file):

    FROM (VALUES (1) UNION ALL SELECT id FROM system.tenants) AS u(id)
   WHERE NOT EXISTS (SELECT 1 FROM system.tenants t WHERE t.id=u.id+1)
     AND NOT EXISTS (SELECT 1 FROM system.tenants t WHERE t.name=$1)

IIUC, if we call this from 2 places at the same time, both will try to create tenants with the same id and will fail, I think the error will be "tenant 7 already exists":

cockroach/pkg/sql/tenant.go

Line 111 in 19382ae

    
           return pgerror.Newf(pgcode.DuplicateObject, "tenant \"%d\"%s already exists", tenID, extra)

even though both callers only passed a name, and those names were unique.
not ideal I guess.
does it make sense to push the id generation down to the query that does the INSERT?

pkg/sql/tenant.go line 254 at r1 (raw file):

	}
	nextID := *row[0].(*tree.DInt)
	return roachpb.MakeTenantID(uint64(nextID)), p.CreateTenantWithID(ctx, uint64(nextID), name)

you probably want to return an empty id if CreateTenantWithID fails, right?

pkg/sql/sem/builtins/builtins.go line 4844 at r1 (raw file):

				return args[0], nil
			},
			Info:       "Creates a new tenant with the provided ID. Must be run by the System tenant.",

nit: ".. with the provided ID and name..."
no what you touched but why not fix..

Code quote:

ID.

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @lidorcarmel)

pkg/sql/tenant.go line 236 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

why a real id on failure and not roachpb.TenantID{} as you do below?

nice catch, my mistake.

pkg/sql/tenant.go line 240 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

I think changing this as you say makes sense. otherwise when debugging we will always need to think about "is id x the same x as the one we deleted yesterday?" and stuff like that. even if it's rare, having an id that was never used before simplifies things.

thinking a bit.. IIUC we allow creating tenants with whatever id the caller wants, right? so if I create a tenant with id MAX-1, that's not going to be great if we change this code to use the max id we ever saw plus one, right? in that case it's not going to work and the existing code is better.

Yeah I think I agree with you about recycling IDs leading to more confusion. Though I cannot claim to have thought about this as much as @knz so I'll defer to him about what we should pursue.

pkg/sql/tenant.go line 244 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

IIUC, if we call this from 2 places at the same time, both will try to create tenants with the same id and will fail, I think the error will be "tenant 7 already exists":

cockroach/pkg/sql/tenant.go

Line 111 in 19382ae

return pgerror.Newf(pgcode.DuplicateObject, "tenant \"%d\"%s already exists", tenID, extra)

even though both callers only passed a name, and those names were unique.
not ideal I guess.
does it make sense to push the id generation down to the query that does the INSERT?

Just thinking out loud but if you run two create_tenant calls with unique names, each one is going to be run in its own planner txn. i.e. all the steps including computing a unique ID and writing to the system.tenants table are done under the scope of p.Txn(). So I believe when the txn that finishes later goes to commit a key with a conflicting PK it will see that someone has already written at a timestamp below what it read when computing the unique ID to assign to the tenant. So this will cause the later txn to error out with a retry error?

I'm not sure I understand how pushing ID generation into the INSERT achieves different behavior, considering the read/write both happen under the same txn.

pkg/sql/tenant.go line 254 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

you probably want to return an empty id if CreateTenantWithID fails, right?

hopefully the caller checks error before using the return type but why risk it, changed

pkg/sql/sem/builtins/builtins.go line 4844 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

nit: ".. with the provided ID and name..."
no what you touched but why not fix..

done.

knz

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

pkg/sql/tenant.go line 244 at r1 (raw file):

IIUC, if we call this from 2 places at the same time, both will try to create tenants with the same id and will fail,

This is not how SQL atomicity work. The two txns would be mutually ordered and would compute differnt IDs.

knz

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

pkg/sql/tenant.go line 244 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

IIUC, if we call this from 2 places at the same time, both will try to create tenants with the same id and will fail,

This is not how SQL atomicity work. The two txns would be mutually ordered and would compute differnt IDs.

Oh wait I misread. This is indeed a problem, Lidor was right.
The original code I provided does the SELECT and the INSERT inside a single statement. that's the goal.

lidorcarmel

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @knz)

pkg/sql/tenant.go line 240 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Yeah I think I agree with you about recycling IDs leading to more confusion. Though I cannot claim to have thought about this as much as @knz so I'll defer to him about what we should pursue.

lgtm. maybe it's worth deprecating the old api? so that ids will only be generated automatically? but that's not really related to this pr.. just a thought.

pkg/sql/tenant.go line 244 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Oh wait I misread. This is indeed a problem, Lidor was right.
The original code I provided does the SELECT and the INSERT inside a single statement. that's the goal.

now I'm confused.. Aditya is right that both statements (SELECT and INSERT) are done within the same txn, so we should be good?

pkg/sql/tenant.go line 254 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

hopefully the caller checks error before using the return type but why risk it, changed

lgtm, hopefully indeed.

adityamaru · 2022-11-09T00:43:56Z

The original code I provided does the SELECT and the INSERT inside a single statement.

I see, can you help me understand how this is different from running the SELECT and INSERT in separate statements under the same txn?

knz

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel)

pkg/sql/tenant.go line 244 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

now I'm confused.. Aditya is right that both statements (SELECT and INSERT) are done within the same txn, so we should be good?

Oh are they the same txn? It wasn't obvious to me. In that case, yes we're good. Sorry for the noise.

knz

@jaylim-crl @jeffswenson Can you check for us how the create_tenant() function is called from the CC control plane?

This PR as-is adds an overload, which means that a prepared statement with the syntax create_tenant($1) and no type hint from the client will not produce an error (that the type is ambiguous). If the CC control plane adds the type hint, we're fine. Otherwise, we may want to strategize this change a bit better.

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

knz

will now produce an error

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

jaylim-crl

@knz the call looks like the following:

	SELECT
		CASE
			WHEN NOT EXISTS(SELECT * FROM system.tenants WHERE id = $1)
			THEN crdb_internal.create_tenant($1)
		END

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

knz · 2022-11-09T14:48:27Z

@adityamaru can you check how the query that jay pasted would behave wrt type checking with your change?

jaylim-crl · 2022-11-09T14:49:01Z

We could also make changes to the CC control plane where necessary, provided that the change is backwards compatible (i.e. working with existing versions).

knz · 2022-11-09T14:51:47Z

ok thanks. The change proposed here would be to modify the query to read ... create_tenant($1:::INT) ... (i.e. add a type annotation). This would be backward-compatible.

adityamaru · 2022-11-09T14:55:35Z

can you check how the query that jay pasted would behave wrt type checking with your change

It would fail! I fixed a few tests where I had to provide a type hint as you suggested. Good catch I didn't think about CC. @jaylim-crl do you think you could push out a fix for this? If not we can think of alternatives.

edit: oh nvm looks like Jeff's already on it 🚀

adityamaru · 2022-11-09T17:15:03Z

@jeffswenson just confirming that with your change merged I'm good to merge this?

jeffswenson · 2022-11-09T17:28:49Z

@jeffswenson just confirming that with your change merged I'm good to merge this?

Yep! You can merge this. The CC change will roll out long before 23.1 is rolled out in production.

adityamaru · 2022-11-09T17:50:51Z

TFTRs!

bors r+

yuzefovich · 2022-11-09T23:45:41Z

Looks like this PR killed the CI and might need to be rebased:

[21:09:56][Run Bazel build] panic: Multiple signatures have oid 2040: [crdb_internal.create_tenant(name: string) -> int to_char(timestamp: timestamp, format: string) -> string]
[21:09:56][Run Bazel build] 
[21:09:56][Run Bazel build] goroutine 1 [running]:
[21:09:56][Run Bazel build] github.com/cockroachdb/cockroach/pkg/sql/sem/builtins.init.3()
[21:09:56][Run Bazel build] 	github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/fixed_oids.go:2057 +0x33
[21:09:58][Run Bazel build] INFO: Elapsed time: 577.322s, Critical Path: 288.01s
[21:09:58][Run Bazel build] INFO: 9698 processes: 621 internal, 3587 local, 5490 processwrapper-sandbox.
[21:09:58][Run Bazel build] FAILED: Build did NOT complete successfully
[21:09:58][Run Bazel build] panic: runtime error: invalid memory address or nil pointer dereference
[21:09:58][Run Bazel build] [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0xadb853]
[21:09:58][Run Bazel build] 
[21:09:58][Run Bazel build] goroutine 35 [running]:
[21:09:58][Run Bazel build] main.(*monitorBuildServer).Finalize(0xc0000389c0)
[21:09:58][Run Bazel build] 	main/pkg/cmd/bazci/bazci.go:244 +0xd3
[21:09:58][Run Bazel build] main.(*monitorBuildServer).PublishBuildToolEventStream(0xc00011eb80?, {0xd90a10, 0xc000382240})
[21:09:58][Run Bazel build] 	main/pkg/cmd/bazci/bazci.go:160 +0xff
[21:09:58][Run Bazel build] google.golang.org/genproto/googleapis/devtools/build/v1._PublishBuildEvent_PublishBuildToolEventStream_Handler({0xbd2780?, 0xc0000389c0}, {0xd8f1c8?, 0xc00038c0d0})
[21:09:58][Run Bazel build] 	google.golang.org/genproto/googleapis/devtools/build/v1/bazel-out/k8-fastbuild/bin/external/go_googleapis/google/devtools/build/v1/build_go_proto_/google.golang.org/genproto/googleapis/devtools/build/v1/publish_build_event.pb.go:816 +0x9f
[21:09:58][Run Bazel build] google.golang.org/grpc.(*Server).processStreamingRPC(0xc00021d180, {0xd90e58, 0xc000185d40}, 0xc00039e240, 0xc000268390, 0x127a380, 0x0)
[21:09:58][Run Bazel build] 	google.golang.org/grpc/external/org_golang_google_grpc/server.go:1542 +0xf5c
[21:09:58][Run Bazel build] google.golang.org/grpc.(*Server).handleStream(0xc00021d180, {0xd90e58, 0xc000185d40}, 0xc00039e240, 0x0)
[21:09:58][Run Bazel build] 	google.golang.org/grpc/external/org_golang_google_grpc/server.go:1624 +0x9ea
[21:09:58][Run Bazel build] google.golang.org/grpc.(*Server).serveStreams.func1.2()
[21:09:58][Run Bazel build] 	google.golang.org/grpc/external/org_golang_google_grpc/server.go:922 +0x98
[21:09:58][Run Bazel build] created by google.golang.org/grpc.(*Server).serveStreams.func1
[21:09:58][Run Bazel build] 	google.golang.org/grpc/external/org_golang_google_grpc/server.go:920 +0x28a
...

bors r-

craig · 2022-11-09T23:45:43Z

Canceled.

This change adds another overload to the `crdb_internal.create_tenant` builtin that only accepts the name the of the tenant to be created. The underlying logic automatically picks the next unique ID based on the existing tenants in the `system.tenants` table. The builtin returns the ID assigned to the newly created tenant or an error if a tenant with the same name already exists. Just like other overloads this buitlin is admin-only and can only be run by the system tenant. Informs: cockroachdb#91235 Release note: None

adityamaru · 2022-11-10T15:28:43Z

pkg/sql/tenant.go

+    FROM (VALUES (1) UNION ALL SELECT id FROM system.tenants) AS u(id)
+   WHERE NOT EXISTS (SELECT 1 FROM system.tenants t WHERE t.id=u.id+1)
+     AND NOT EXISTS (SELECT 1 FROM system.tenants t WHERE t.name=$1)
+   ORDER BY id LIMIT 1


note, i had to add an order by clause so that we get the first available ID.

adityamaru · 2022-11-10T16:51:33Z

bors r+

craig · 2022-11-10T18:17:30Z

Build succeeded:

Bazel Essential CI (Cockroach)

adityamaru requested review from a team, knz and lidorcarmel November 8, 2022 16:27

adityamaru commented Nov 8, 2022

View reviewed changes

pkg/sql/tenant.go Show resolved Hide resolved

lidorcarmel suggested changes Nov 8, 2022

View reviewed changes

adityamaru force-pushed the auto-id-gen branch from 283d123 to cdb89a1 Compare November 8, 2022 21:38

adityamaru commented Nov 8, 2022

View reviewed changes

adityamaru requested a review from lidorcarmel November 8, 2022 21:39

knz reviewed Nov 8, 2022

View reviewed changes

lidorcarmel approved these changes Nov 8, 2022

View reviewed changes

adityamaru requested a review from knz November 9, 2022 00:44

adityamaru force-pushed the auto-id-gen branch from cdb89a1 to 1338646 Compare November 9, 2022 14:08

adityamaru requested a review from a team as a code owner November 9, 2022 14:08

knz reviewed Nov 9, 2022

View reviewed changes

jaylim-crl reviewed Nov 9, 2022

View reviewed changes

adityamaru mentioned this pull request Nov 9, 2022

sql: introduce CREATE TENANT syntax #91595

Merged

adityamaru force-pushed the auto-id-gen branch from 1338646 to b2c3202 Compare November 10, 2022 13:58

adityamaru force-pushed the auto-id-gen branch from b2c3202 to 8d15a3a Compare November 10, 2022 15:21

adityamaru commented Nov 10, 2022

View reviewed changes

craig bot merged commit 58e91e2 into cockroachdb:master Nov 10, 2022

adityamaru deleted the auto-id-gen branch November 10, 2022 21:58

Conversation

adityamaru commented Nov 8, 2022

Uh oh!

cockroach-teamcity commented Nov 8, 2022

Uh oh!

Uh oh!

lidorcarmel left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

lidorcarmel left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Nov 9, 2022

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

jaylim-crl left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Nov 9, 2022

Uh oh!

jaylim-crl commented Nov 9, 2022

Uh oh!

knz commented Nov 9, 2022

Uh oh!

adityamaru commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityamaru commented Nov 9, 2022

Uh oh!

jeffswenson commented Nov 9, 2022

Uh oh!

adityamaru commented Nov 9, 2022

Uh oh!

yuzefovich commented Nov 9, 2022

Uh oh!

craig bot commented Nov 9, 2022

Uh oh!

adityamaru Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Nov 10, 2022

Uh oh!

craig bot commented Nov 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adityamaru commented Nov 9, 2022 •

edited

Loading