opt: check UDFs and UDTs by name when checking metadata dependencies #96045
Conversation
|
This change is |
DrewKimball
requested review from
chengxiong-ruan
and removed request for
a team
|
Hold off on reviewing this for a bit - it should be possible to fix #93321 as well. |
DrewKimball
force-pushed
the
drop-function
branch
from
11eee0e
to
d2eaea5
Compare
|
Alright, this is RFAL. |
There was a problem hiding this comment.
This looks good. My comment is nit-picky and more of a feeling than anything else. A lot of code in tree makes me feel uneasy with its unprincipled mutability etc, so there's nothing you could probably do that would make me feel good.
Feel free to wave me off if you like what's there.
pkg/sql/sem/tree/function_name.go
Outdated
| @@ -63,6 +63,9 @@ type FunctionReferenceResolver interface { | |||
| // ResolvableFunctionReference implements the editable reference call of a | |||
| // FuncExpr. | |||
| type ResolvableFunctionReference struct { | |||
| // CallsiteName is the original UnresolvedName used at the function callsite. | |||
| // It may be unset if the function is unresolved. | |||
| CallsiteName *UnresolvedName | |||
There was a problem hiding this comment.
Something about this field name bothers me. I think your approach here is sound. Also, I wonder if we should make this field unexported and have the act of resolving which rewrites this in place to do the swapping out into the field. Then have a method which uses the unexported field if it's populated and otherwise delegates to the underlying function reference.
There was a problem hiding this comment.
This is great!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @DrewKimball)
pkg/sql/opt/metadata.go line 355 at r2 (raw file):
} } // handleUndefined swallows "undefined" and "dropped object errors, since
nit: s/"dropped object/"dropped" object
pkg/sql/opt/metadata.go line 434 at r2 (raw file):
} if name == nil { panic(errors.AssertionFailedf("attempted to add UDF with nil name"))
Is there a way to error gracefully instead of panic here? Better to not panic if we can.
pkg/sql/opt/metadata.go line 439 at r2 (raw file):
if md.userDefinedFunctions[i].overload == fun && md.userDefinedFunctions[i].name == name { // This is a duplicate. break
Should this be a return? Right now it looks like we will always add the UDF to the metadata whether we found a duplicate or not.
Is there a test case that covers duplicates?
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status:
pkg/sql/opt/metadata.go line 377 at r2 (raw file):
for i := range md.userDefinedFunctions { dep := &md.userDefinedFunctions[i] toCheck, err := optCatalog.ResolveFunction(ctx, dep.name, &evalCtx.SessionData().SearchPath)
Can we resolve the overload by ID rather than by name? I think that'd be more robust than using MatchOverload.
pkg/sql/opt/metadata.go line 430 at r2 (raw file):
// this query. func (md *Metadata) AddUserDefinedFunc(fun *tree.Overload, name *tree.UnresolvedName) { if !fun.IsUDF {
We now set Overload.IsUDF to true for some builtins that use the UDF mechanisms. This is some tech debt that I think is time to address. I think we'll need two different booleans:
Overload.IsUDFcan mean what it says - is the function user-defined.Overload.HasBody(or some better name you come up with) can mean that an Overload's implementation is defined in theBodyfield.
We can't rely on an empty Body for the second point in order to be completely consistent with Postgres because UDFs with empty bodies are allowed.
Might be good to make this refactor in a separate PR/commit.
pkg/sql/opt/metadata.go line 437 at r2 (raw file):
} for i := range md.userDefinedFunctions { if md.userDefinedFunctions[i].overload == fun && md.userDefinedFunctions[i].name == name {
Why do we need to check the name here?
pkg/sql/sem/tree/function_name.go line 68 at r2 (raw file):
Previously, ajwerner wrote…
Something about this field name bothers me. I think your approach here is sound. Also, I wonder if we should make this field unexported and have the act of resolving which rewrites this in place to do the swapping out into the field. Then have a method which uses the unexported field if it's populated and otherwise delegates to the underlying function reference.
If CheckDependencies resolves the function by ID/OID, then we shouldn't need this, right?
DrewKimball
force-pushed
the
drop-function
branch
from
d2eaea5
to
8b094c8
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/metadata.go line 355 at r2 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
nit: s/"dropped object/"dropped" object
Done.
pkg/sql/opt/metadata.go line 377 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Can we resolve the overload by ID rather than by name? I think that'd be more robust than using
MatchOverload.
We have to use name in case the name now resolves to a different overload - for example, in logic_test/udf the re-created sequence will return 5 instead of 1 if we don't properly handle this. It also wouldn't handle the database being switched.
pkg/sql/opt/metadata.go line 430 at r2 (raw file):
I've refactored in a new commit. I'd appreciate some extra scrutiny for the cases where IsUDF is checked vs HasSQLBody - I left some as IsUDF because they seem to care only about whether the function was user-defined, not whether it uses the SQL string in its definition.
We can't rely on an empty Body for the second point in order to be completely consistent with Postgres because UDFs with empty bodies are allowed.
That case is only when IsUDF is true, right? So it should be enough to check IsUDF || Body != "". I've added a method to Overload to handle this. If you think it's better to add HasBody, I'm fine with that as well.
pkg/sql/opt/metadata.go line 434 at r2 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Is there a way to error gracefully instead of panic here? Better to not panic if we can.
I don't think there's a way to propagate an error without plumbing it through the optbuilder. I think the panic should be caught be the optimizer, anyway.
pkg/sql/opt/metadata.go line 437 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Why do we need to check the name here?
We can't de-duplicate fully if a function is referred to in different ways (say, db.schema.func vs func), since that can change how a function is resolved in a different context (like if the database name is changed). I've changed the field from name to reference and added a comment to try and clarify this a bit. Also expanded the tests.
pkg/sql/opt/metadata.go line 439 at r2 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Should this be a return? Right now it looks like we will always add the UDF to the metadata whether we found a duplicate or not.
Is there a test case that covers duplicates?
Good catch, it should be a return. It's difficult to test directly whether the de-duplication is working (and I'm not sure it's worth it since it's only a performance issue), but I'll expand the test cases a bit.
pkg/sql/sem/tree/function_name.go line 68 at r2 (raw file):
Something about this field name bothers me.
Changed it to Reference - does that make more sense to you?
Also, I wonder if we should make this field unexported and have the act of resolving which rewrites this in place to do the swapping out into the field. Then have a method which uses the unexported field if it's populated and otherwise delegates to the underlying function reference.
I like this idea. Done.
DrewKimball
requested review from
jayshrivastava
and removed request for
a team and
jayshrivastava
There was a problem hiding this comment.
Don't forget to update the PR description with the new commit(s).
Reviewed 17 of 17 files at r3, 4 of 9 files at r4, all commit messages.
Reviewable status:
pkg/sql/delegate/show_function.go line 42 at r3 (raw file):
var udfSchema string for _, o := range fn.Overloads { if o.HasSQLBody() {
nit: Can we leave this as IsUDF? It'd be odd to have SHOW CREATE FUNCTION work on just some built-ins.
pkg/sql/opt/metadata.go line 377 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
We have to use name in case the name now resolves to a different overload - for example, in
logic_test/udfthe re-created sequence will return5instead of1if we don't properly handle this. It also wouldn't handle the database being switched.
We talked IRL - there may be a way to use IDs instead of names.
pkg/sql/opt/metadata.go line 430 at r2 (raw file):
That case is only when IsUDF is true, right? So it should be enough to check IsUDF || Body != "". I've added a method to Overload to handle this. If you think it's better to add HasBody, I'm fine with that as well.
Great idea - I like it.
pkg/sql/opt/metadata.go line 240 at r4 (raw file):
if len(from.userDefinedFunctions) > 0 { for i := range from.userDefinedFunctions {
nit: make a new slice with the exact capacity if md.userDefinedFunctions is nil.
DrewKimball
force-pushed
the
drop-function
branch
from
8b094c8
to
50f798e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/delegate/show_function.go line 42 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: Can we leave this as
IsUDF? It'd be odd to haveSHOW CREATE FUNCTIONwork on just some built-ins.
Done.
pkg/sql/opt/metadata.go line 377 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
We talked IRL - there may be a way to use IDs instead of names.
I've refactored the dependency checking to also check the schemas that are used to resolve each object, as well as the current database if it was used during resolution. This allows us to check only IDs for all schema objects.
pkg/sql/opt/metadata.go line 240 at r4 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit:
makea new slice with the exact capacity ifmd.userDefinedFunctionsisnil.
Done.
DrewKimball
force-pushed
the
drop-function
branch
from
50f798e
to
ce9c318
Compare
DrewKimball
force-pushed
the
drop-function
branch
from
ce9c318
to
b30310c
Compare
|
This should be ready for another look now. |
DrewKimball
force-pushed
the
drop-function
branch
from
b30310c
to
0cf3150
Compare
Scratch that, looks like I have some more test failures to iron out. |
|
TFYRs! bors r+ |
|
Build succeeded: |
This commit removes a TODO that was addressed by cockroachdb#96045. Release note: None
101937: catalog: utilize scans for a large number of descriptors r=fqazi a=fqazi Previously, the catalog descriptor would fetch descriptors via point lookups using Get when scanning large batches of descriptors. This was further extended to also look up ZoneConfigs and comments in a similar way. Recently, we we started seeing regression on the Django test suite involving the pg_catalog tables, which tend to do read large number of descriptors, likely linked to extra overhead linked to both comments and zone configs in 23.1. To address this, this patch ill now start using scans for runs of descriptor IDs for batch scans which reduces the overall cost of fetching a large number of descriptors hiding this cost. Fixes: #100871 Release note: None 101943: build,release: provide way to inject build tag override, use in MAPB r=rail a=rickystewart When we build nightly builds, we build them in "release" configuration. Because we do this, the build tag reported by these binaries from `cockroach version` has been identical to that of an actual release binary, which is very confusing. Here we update the script to build these nightlies (`make-and-publish-build-artifacts.sh`) to inject an appropriate, identifiable build tag. It is probably wrong that we are building these nightlies as if they were "releases". We'll follow up with work to fix this and refine the build tags further. Closes #100532. Epic: None Release note (build change): Update reported `Build Tag` for nightly (non-release) builds 101944: sem/tree: remove TODO r=mgartner a=mgartner This commit removes a TODO that was addressed by #96045. Epic: None Release note: None 101952: ui: fix linegraph render for large clusters r=zachlite a=zachlite We were feeding `Math.min` and `Math.max` very large arrays (length equal to ~10e7). These functions take variadic arguments, and the runtime can't handle that many arguments. As a result we blow the stack, causing uncaught range errors. Instead, we'll use lodash min and max which are not variadic. Resolves #101377 Epic: None Release note: None Reproduction and Fix demonstrated on the command line: <img width="722" alt="Screenshot 2023-04-20 at 4 56 41 PM" src="https://user-images.githubusercontent.com/5423191/233486016-fd740a98-9a0d-4fef-a6d8-67e9cb4e318e.png"> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com> Co-authored-by: Ricky Stewart <rickybstewart@gmail.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Zach Lite <zach@cockroachlabs.com>
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#96045 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
102405: opt: check privileges after data sources in memo staleness check r=DrewKimball a=DrewKimball Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before #96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after #96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes #102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables. Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before #96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after #96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes #102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
Previously, the `opt.Metadata` staleness check would check the access privileges for a data source immediately after checking that the data source still resolves as it originally did. This meant that the privilege check for one data source could occur before checking staleness of other data sources. Before cockroachdb#96045 this wasn't a problem, since these checks would happen in the order in which the data sources were resolved. But after cockroachdb#96045, the data sources were stored in a map instead of a slice, which made the order in which the checks are performed nondeterministic. This change in behavior can cause queries to return insufficient privilege errors even when the current user has access privileges on all data sources referenced by the query. This is because the query may *implicitly* reference a table by ID if there is a foreign key relation from an explicitly referenced data source. If the database is changed, this ID reference will still resolve the same, but if the user is changed to one without privileges on that table, the staleness check will result in an error. This wasn't a problem before because the referencing table would always be checked first, causing the staleness check to finish without an error. This patch fixes the bug by moving the privilege checks until after all data source resolution checks have succeeded. Fixes cockroachdb#102375 Release note (bug fix): Fixed a bug introduced in versions 22.1.19, 22.2.8, and pre-release versions of 23.1 that could cause queries to return spurious insufficient privilege errors. For the bug to occur, two databases would need to have duplicate tables each with a foreign key reference to another table. The error would then occur if the same SQL string was executed against both databases concurrently by users that have privileges over only one of the tables.
tree: distinguish UDFs and builtins that use a SQL string body
This refactor changes the meaning of the
Overload.IsUDFfield tobe true only for user-defined functions - meaning those created using
CREATE FUNCTION. This is contrasted with builtin functions that aredefined with a SQL string set in
Overload.Body. Logic that cares onlyabout user-defined functions can continue checking
Overload.IsUDF,while cases that deal with the SQL body should use
Overload.HasSQLBody().Note that it is insufficient to check whether
Overload.Bodyis emptybecause it is possible to define a UDF with an empty body.
opt: refactor metadata dependency tracking
This commit performs some refactoring for the way data source objects
are tracked in
opt.Metadatain order to make future changes to UDTand UDF dependency tracking easier. More particularly, UDTs and UDFs
will be able to re-resolve any references by name. This is necessary
in order to handle cases where changes to the search-path cause names
in the query to resolve to different objects.
opt: track UDT references by name in the Metadata
Previously, it was possible for invalid queries to be kept in the cache
after a schema change, since user-defined types were tracked only by OID.
This missed cases where the search path changed which object a name in
the query resolved to (or whether it resolved at all).
This patch fixes this behavior by tracking UDT references by name, similar
to what was already done for data sources. This ensures that the query
staleness check doesn't miss changes to the search path.
opt: track UDFs in the Metadata
This patch adds tracking for user-defined functions in
opt.Metadata.This ensures that the query cache will be correctly invalidated after
a schema change or search-path change that could change the query's
semantics, or cause it to error.
Fixes #95214
Fixes #93082
Fixes #93321
Fixes #96674
Release note (bug fix): Fixed a bug that could prevent a cached query from being invalidated when a UDF or UDT referenced by that query was altered or dropped, or when the schema or database of a UDF or UDT was altered or dropped.