sql/catalog: resolve temp schemas from other sessions by ID#165395
sql/catalog: resolve temp schemas from other sessions by ID#165395trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
😎 Merged successfully - details. |
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
One concern with this change
@fqazi reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss).
pkg/sql/catalog/nstree/catalog.go line 189 at r1 (raw file):
// temporary schemas from other sessions, which have namespace entries but // no descriptors. func (c Catalog) LookupNamespaceEntryByID(id descpb.ID) NamespaceEntry {
This does a full search of the namespaces read by the transaction each getDescriptorsByID. Could we some how make this cheaper, if we need to enhance the cache / catalog read it may be worthwhile especially with a large number of descriptors.
Temporary schemas have no descriptor in system.descriptor — only namespace entries in system.namespace. The by-ID resolution path (lookupTemporary) only checked the current session's data via TemporarySchemaProvider, so temp schemas from other sessions couldn't be resolved by ID even though they could be found by name. This caused failures when resolving a table from another session's temp schema, then trying to resolve its parent schema by ID. A workaround existed in pg_catalog.go that iterated all schemas to find the match. Fix this by enhancing the by-ID resolution path to check the catalog reader's cached namespace entries as a fallback when the session-local check returns nil. Also fix IsDescIDKnownToNotExist to not incorrectly report temp schema IDs as non-existent after ScanAll (since they have namespace entries but no descriptors). With these fixes, the pg_catalog.go workaround is no longer needed and is removed. Resolves: cockroachdb#97822 Release note (bug fix): Fixed a bug where temporary tables created in one session could fail to appear in pg_catalog queries from another session because the parent temporary schema could not be resolved by ID. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
rafiss
force-pushed
the
resolve-temp-schema
branch
from
3b96f8b to
3fda94f
Compare
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
@rafiss made 1 comment.
Reviewable status:
pkg/sql/catalog/nstree/catalog.go line 189 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
This does a full search of the namespaces read by the transaction each getDescriptorsByID. Could we some how make this cheaper, if we need to enhance the cache / catalog read it may be worthwhile especially with a large number of descriptors.
done
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
@rafiss Nice work!
@fqazi reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:
|
/trunk merge |
3 participants
Temporary schemas have no descriptor in system.descriptor — only namespace entries in system.namespace. The by-ID resolution path (lookupTemporary) only checked the current session's data via TemporarySchemaProvider, so temp schemas from other sessions couldn't be resolved by ID even though they could be found by name.
This caused failures when resolving a table from another session's temp schema, then trying to resolve its parent schema by ID. A workaround existed in pg_catalog.go that iterated all schemas to find the match.
Fix this by enhancing the by-ID resolution path to check the catalog reader's cached namespace entries as a fallback when the session-local check returns nil. Also fix IsDescIDKnownToNotExist to not incorrectly report temp schema IDs as non-existent after ScanAll (since they have namespace entries but no descriptors). With these fixes, the pg_catalog.go workaround is no longer needed and is removed.
Resolves: #97822
Release note (bug fix): Fixed a bug where temporary tables created in one session could fail to appear in pg_catalog queries from another session because the parent temporary schema could not be resolved by ID.