sql: avoid looking up empty-string object names #58674
Conversation
|
This change is |
pkg/sql/sem/tree/name_resolution.go
Outdated
| // database set. Therefore, we test this even if curDb == "", as long as the | ||
| // schema name is for a virtual schema. | ||
|
|
||
| if _, isVirtualSchema := sessiondata.VirtualSchemaNames[scName]; isVirtualSchema && curDb == "" { |
There was a problem hiding this comment.
i'm confused why we have the curDb == "" check here, we want to check if curDb != "" too right?
There was a problem hiding this comment.
yeah i have a mistake here. we should use the existing name resolution logic if curDb != "" || (curDb == "" && isVirtualSchema)
Sometimes the current DB is "". Normally we wouldn't try to resolve this name, but if there's a mistake in name resolution logic, this check is a simple way to avoid a KV lookup when we don't need it. Release note: None
rafiss
force-pushed
the
fix-empty-name-lookups
branch
from
002cf4a to
45fb9cf
Compare
| @@ -624,6 +625,7 @@ func TestResolveTablePatternOrName(t *testing.T) { | |||
| {`kv`, ``, mpath("public", "pg_catalog"), true, ``, ``, ``, `prefix or object not found`}, | |||
| {`pg_tables`, ``, mpath("public", "pg_catalog"), true, `pg_tables`, `"".pg_catalog.pg_tables`, `.pg_catalog[0]`, ``}, | |||
| {`pg_tables`, ``, mpath(), true, `pg_tables`, `"".pg_catalog.pg_tables`, `.pg_catalog[0]`, ``}, | |||
| {`system.users`, ``, mpath(), true, `system.public.users`, `system.public.users`, `system.public[0]`, ``}, | |||
There was a problem hiding this comment.
system isn't a virtual table is it? should we also add a test with a virtual schema with no currdb set, or is this the wrong place for it? can we add a logic test also?
There was a problem hiding this comment.
yeah it's not virtual, so actually, this test i added works before and after my PR. Not a great test i suppose then :P
(the reason i added it is because querying system.users with a curDB of "" is what the internal executor does, so just wanted to make sure I didn't break that.)
but yeah adding more vtable tests and logic tests is a good call
There was a problem hiding this comment.
actually L627 and L654-L658 seems like good tests. anything else in particular you want to test on that front? i'll still add logic tests
rafiss
force-pushed
the
fix-empty-name-lookups
branch
from
45fb9cf to
a0fc02d
Compare
The internal executor has a current DB of "", and it also executes against queries against system tables using names like `system.users`. The name resolution logic turned this into a lookup of "".system.users, which was not desired. Now we explicitly only allow the "" DB name if the schema name corresponds to a virtual schema. Release note: none
rafiss
force-pushed
the
fix-empty-name-lookups
branch
from
a0fc02d to
16d0f7b
Compare
|
tftr! bors r=otan |
|
Build succeeded: |
sql: fix name resolution to avoid empty-name db lookups
The internal executor has a current DB of "", and it also executes
against queries against system tables using names like
system.users.The name resolution logic turned this into a lookup of "".system.users,
which was not desired.
Now we explicitly only allow the "" DB name if the schema name
corresponds to a virtual schema.
Release note: None
sql/catalog: avoid KV lookup for "" name
Sometimes the current DB is "". Normally we wouldn't try to resolve this
name, but if there's a mistake in name resolution logic, this check is a
simple way to avoid a KV lookup when we don't need it.
Release note: None