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: use SQL sequence as DescIDGenerator for non-system tenants #48513

Closed
nvanbenschoten opened this issue May 6, 2020 · 0 comments
Closed
Assignees
Labels
A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented May 6, 2020

Relates to #47904.

keys.DescIDGenerator is currently the only non-SQL key (i.e. no table ID prefix) that non-system tenants in a multi-tenant cluster will need in their keyspace.

This presents an interesting conundrum - if we want a per-tenant descriptor ID generator, where do we store it? Do we break down and encode a single non-SQL key under tenant ID-prefixed keys? This seems like a mistake.

Instead, we should explore pulling this DescIDGenerator into the SQL-domain properly, but exposing it as a sequence. As it turns out, sequences and the DescIDGenerator already have the exact same value encoding (they both use kv.IncrementValRetryable). If we had support for sequences at the time that the DescIDGenerator was added, we probably would have used them.

Proposal:

  • create a new DescIDSequence with an ID of 7 for all non-system tenants.
  • decide whether to use this key or the existing keys.DescIDGenerator when creating the key and when incrementing the key.
  • optionally migrate keys.DescIDGenerator over.

Extended proposal:

  • clear out and re-use ID 2, which has been vacated with the removal of the DeprecatedNamespaceTable. This is a bit more work, but has the elegant effect of placing the ID generator at the beginning of the ID space.
  • optionally migrate keys.DescIDGenerator over. The timing of this is not pressing, so it could wait out any migration needed for the existing keys in the DeprecatedNamespaceTable.

Note that using ID 7 and placing this sequence in the SystemConfig span is not a concern, because this will range of keys will no longer be gossiped once #47150 is addressed.

@nvanbenschoten nvanbenschoten self-assigned this May 6, 2020
@blathers-crl blathers-crl bot added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label May 6, 2020
@cockroachdb cockroachdb deleted a comment from blathers-crl bot May 6, 2020
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 13, 2020
Fixes cockroachdb#48513.

This commit creates a new SQL sequence called "descriptor_id_seq" that
is used by all non-system tenants to allocate descriptor IDs. This is
necessary because non-system tenants should not be using the global
`DescIDGenerator` key to allocate IDs.

The next commit includes tests that exercise the MetadataSchema portion
of this change.
@craig craig bot closed this as completed in 027f6e3 May 14, 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-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

1 participant