Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: ALTER TABLE statement issued by tenant cannot find relation #49318

Closed
asubiotto opened this issue May 20, 2020 · 4 comments · Fixed by #49784
Closed

sql: ALTER TABLE statement issued by tenant cannot find relation #49318

asubiotto opened this issue May 20, 2020 · 4 comments · Fixed by #49784
Assignees
Labels
A-multitenancy Related to multi-tenancy C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@asubiotto
Copy link
Contributor

Modifying the logic test harness to set an explicit tenant ID:

diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go
index 67d805b16f..9a7490c1a7 100644
--- a/pkg/server/testserver.go
+++ b/pkg/server/testserver.go
@@ -527,7 +527,14 @@ func testSQLServerArgs(ts *TestServer) sqlServerArgs {
 // StartTenant starts a SQL tenant communicating with this TestServer.
 func (ts *TestServer) StartTenant() (addr string, _ error) {
        ctx := context.Background()
+       if _, err := ts.InternalExecutor().(*sql.InternalExecutor).Exec(
+               ctx, "testserver-create-tenant", nil /* txn */, "SELECT crdb_internal.create_tenant(10)",
+       ); err != nil {
+               return "", err
+       }
+
        args := testSQLServerArgs(ts)
+       args.tenantID = roachpb.MakeTenantID(10)

And running the following test:

# LogicTest: 3node-tenant

statement ok
CREATE TABLE x (x int)

statement ok
INSERT INTO x VALUES (1)

query I
SELECT * FROM x
----
1

statement error operation is unsupported
ALTER TABLE x CONFIGURE ZONE USING num_replicas = 123

Fails on the ALTER TABLE statement.

        expected:
        operation is unsupported
        got:
        pq: relation "x" does not exist

cc @tbg @nvanbenschoten

@asubiotto asubiotto added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-multitenancy Related to multi-tenancy A-multitenancy-blocker labels May 20, 2020
@tbg
Copy link
Member

tbg commented May 22, 2020

@asubiotto You bring up an interesting point. We should be running the logic tests as a non-system tenant. (The standard run is basically how we will use the system tenant - built into KV).

Would you mind making that change? I think we'd pass a tenant id into StartTenant (and testSQLServerArgs and then use pretty much your code. The logic test usage could just hard-code 10.

@tbg
Copy link
Member

tbg commented May 22, 2020

I don't know how it explains this particular error but I think the problem will be similar to the use of the SystemCodec here:

if zoneVal, err := getKey(config.MakeZoneKey(id)); err != nil {

I'm pretty sure you can have this code called if you're not the system tenant, in which case the lookup will currently work (but not quite right) and in the future it will fail as the KV layer will reject it.

@tbg
Copy link
Member

tbg commented May 22, 2020

Ah it's probably here

dbID, zone, _, _, err := getZoneConfig(uint32(tableDesc.ParentID), getKey, false /* getInheritedDefault */)

@asubiotto
Copy link
Contributor Author

Just a note that a lot of logic tests are failing due to this so I'll hold off on triaging them until this is fixed.

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jun 2, 2020
Fixes cockroachdb#49318.
Fixes cockroachdb#49445.
Progress towards cockroachdb#48123.
Informs cockroachdb#48774.

This commit introduces a new pseudo object ID in the system tenant's
namespace called "tenants". Like "liveness" and "timeseries" before it,
the pseudo object allows zone configurations to be applied to
pseudo-objects that do not live directly in the system tenant's SQL
keyspace. In this case, the "tenants" zone allows zone configurations to
be set by the system tenant and applied to all other tenants in the
system. There may come a time when we want secondary tenants to have
more control over their zone configurations, but that will require a
much larger change to the zone config structure and UX as a whole.

While making this change, we rationalize the rest of zone configuration
handling and how it relates to multi-tenancy. Now that secondary tenant
ranges have a zone config to call their own, we can make sense of calls
from SQL into the zone configuration infrastructure. We gate off calls
that don't make sense for secondary tenants and clean up hooks in SQL
that handle zone config manipulation.

All of this works towards a good cause - we eliminate the remaining uses
of `keys.TODOSQLCodec` from `pkg/sql/...` and `pkg/config/...`, bringing
us a big step closer towards being able to remove the placeholder and
close cockroachdb#48123.

This work also reveals that in order to address cockroachdb#48774, we need to be
able to determine split points from the SystemConfig. This makes it very
difficult to split on secondary tenant object (e.g. table) boundaries.
However, it makes it straightforward to split on secondary tenant
keysapce boundaries. This is already what we were thinking (see cockroachdb#47907),
so out both convenience and desire, I expect that we'll follow this up
with a PR that splits Ranges only at secondary tenant boundaries -
placing the overhead of an otherwise empty tenant at only a single Range
and a few KBs of data.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jun 3, 2020
Fixes cockroachdb#49318.
Fixes cockroachdb#49445.
Progress towards cockroachdb#48123.
Informs cockroachdb#48774.

This commit introduces a new pseudo object ID in the system tenant's
namespace called "tenants". Like "liveness" and "timeseries" before it,
the pseudo object allows zone configurations to be applied to
pseudo-objects that do not live directly in the system tenant's SQL
keyspace. In this case, the "tenants" zone allows zone configurations to
be set by the system tenant and applied to all other tenants in the
system. There may come a time when we want secondary tenants to have
more control over their zone configurations, but that will require a
much larger change to the zone config structure and UX as a whole.

While making this change, we rationalize the rest of zone configuration
handling and how it relates to multi-tenancy. Now that secondary tenant
ranges have a zone config to call their own, we can make sense of calls
from KV into the zone configuration infrastructure. We gate off calls
that don't make sense for secondary tenants and clean up hooks in SQL
that handle zone config manipulation.

All of this works towards a good cause - we eliminate the remaining uses
of `keys.TODOSQLCodec` from `pkg/sql/...` and `pkg/config/...`, bringing
us a big step closer towards being able to remove the placeholder and
close cockroachdb#48123.

This work also reveals that in order to address cockroachdb#48774, we need to be
able to determine split points from the SystemConfig. This makes it very
difficult to split on secondary tenant object (e.g. table) boundaries.
However, it makes it straightforward to split on secondary tenant
keysapce boundaries. This is already what we were thinking (see cockroachdb#47907),
so out both convenience and desire, I expect that we'll follow this up
with a PR that splits Ranges only at secondary tenant boundaries -
placing the overhead of an otherwise empty tenant at only a single Range
and a few KBs of data.
craig bot pushed a commit that referenced this issue Jun 4, 2020
49784: config: introduce pseudo "tenants" zone r=nvanbenschoten a=nvanbenschoten

Fixes #49318.
Fixes #49445.
Progress towards #48123.
Informs #48774.

This commit introduces a new pseudo object ID in the system tenant's namespace called "tenants". Like "liveness" and "timeseries" before it, the pseudo object allows zone configurations to be applied to pseudo-objects that do not live directly in the system tenant's SQL keyspace. In this case, the "tenants" zone allows zone configurations to be set by the system tenant and applied to all other tenants in the system. There may come a time when we want secondary tenants to have more control over their zone configurations, but that will require a much larger change to the zone config structure and UX as a whole.

While making this change, we rationalize the rest of zone configuration handling and how it relates to multi-tenancy. Now that secondary tenant ranges have a zone config to call their own, we can make sense of calls from SQL into the zone configuration infrastructure. We gate off calls that don't make sense for secondary tenants and clean up hooks in SQL that handle zone config manipulation.

All of this works towards a good cause - we eliminate the remaining uses of `keys.TODOSQLCodec` from `pkg/sql/...` and `pkg/config/...`, bringing us a big step closer towards being able to remove the placeholder and close #48123.

This work also reveals that in order to address #48774, we need to be able to determine split points from the SystemConfig. This makes it very difficult to split on secondary tenant object (e.g. table) boundaries. However, it makes it straightforward to split on secondary tenant keysapce boundaries. This is already what we were thinking (see #47907), so out both convenience and desire, I expect that we'll follow this up with a PR that splits Ranges only at secondary tenant boundaries - placing the overhead of an otherwise empty tenant at only a single Range and a few KBs of data.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig craig bot closed this as completed in 6953182 Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants