-
Notifications
You must be signed in to change notification settings - Fork 4k
sql: more routine dependency fixes #158935
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
base: master
Are you sure you want to change the base?
Conversation
Previously, we did not track dependencies on columns in the SET clause of an UPDATE statement while creating a routine. This meant that it was possible to break a routine by dropping such a column. This commit fixes the bug by adding target columns that are explicitly referenced by an UPDATE statement to the routine dependencies. The new behavior is gated behind the preexisting variable `use_improved_routine_dependency_tracking`. Fixes cockroachdb#158898 Release note (bug fix): Fixed a bug that allowed a column to be dropped from its table even if a routine referenced that column in the SET clause of an UPDATE statement. This fix only applies to newly-created routines. In versions prior to v25.3, the fix must be enabled by setting the session variable `use_improved_routine_dependency_tracking`.
This commit pulls out some re-arranging of the `triggers` logic test to avoid a confusing diff in the following commmit. Epic: None Release note: None
Previously, there were several cases where routines would add unnecessary column dependencies: * Columns referenced by a computed column expression would be added to the list of columns referenced by the routine. * BEFORE triggers would confuse the logic used to distinguish explicit from implicit INSERT columns, because triggers change the column IDs to be inserted. This could cause the routine to add implicitly referenced columns to its dependencies. * Dependencies from statements within a trigger would transitively apply to a routine that invoked trigger on a mutation. This commit prevents newly-created routines from adding unnecessary column dependencies in those cases, gated behind the preexisting session setting `use_improved_routine_dependency_tracking`. Fixes cockroachdb#158154 Release note (bug fix): Fixed a bug that caused routines to prevent dropping more columns than necessary, most notably columns referenced by computed column expressions. The fix is gated behind the session setting `use_improved_routine_dependency_tracking`, which is off by default prior to v25.3.
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
mgartner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think the safe thing to do is make a new session setting given that use_improved_routine_dependency_tracking is already on by default in 25.3.
@mgartner reviewed 3 of 3 files at r1, 1 of 1 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @ZhouXing19)
pkg/sql/logictest/testdata/logic_test/udf_update line 382 at r1 (raw file):
ALTER TABLE table_drop DROP COLUMN c; statement error pgcode 2BP01 pq: cannot drop column "b" because function "f_update" depends on it
Looks like we were already handling the case were column is referenced on the RHS of the assignment, correct? It might be good to make the release note and commit message a little more specific about this.
pkg/sql/opt/optbuilder/mutation_builder.go line 80 at r1 (raw file):
// explicitTargetColOrds contains the ordinals of the columns in targetColSet // that were explicitly specified by the user, e.g. in the target column list // of an INSERT or the SET clause of an UPDATE. This is used to determine
Was there a reproducible bug in INSERT too? Or just UPDATE?
sql: track SET columns in routine dependencies
Previously, we did not track dependencies on columns in the SET clause
of an UPDATE statement while creating a routine. This meant that it was
possible to break a routine by dropping such a column. This commit fixes
the bug by adding target columns that are explicitly referenced by an
UPDATE statement to the routine dependencies. The new behavior is gated
behind the preexisting variable
use_improved_routine_dependency_tracking.Fixes #158898
Release note (bug fix): Fixed a bug that allowed a column to be dropped
from its table even if a routine referenced that column in the SET clause
of an UPDATE statement. This fix only applies to newly-created routines.
In versions prior to v25.3, the fix must be enabled by setting the session
variable
use_improved_routine_dependency_tracking.sql/logictest: rearrange trigger logic test
This commit pulls out some re-arranging of the
triggerslogic testto avoid a confusing diff in the following commmit.
Epic: None
Release note: None
sql: avoid unnecessary column dependencies in routines
Previously, there were several cases where routines would add unnecessary
column dependencies:
the list of columns referenced by the routine.
from implicit INSERT columns, because triggers change the column IDs
to be inserted. This could cause the routine to add implicitly referenced
columns to its dependencies.
to a routine that invoked trigger on a mutation.
This commit prevents newly-created routines from adding unnecessary
column dependencies in those cases, gated behind the preexisting
session setting
use_improved_routine_dependency_tracking.Fixes #158154
Release note (bug fix): Fixed a bug that caused routines to prevent
dropping more columns than necessary, most notably columns referenced
by computed column expressions. The fix is gated behind the session
setting
use_improved_routine_dependency_tracking, which is off bydefault prior to v25.3.