sql: reject volatility weakening when IMMUTABLE-required dependent exists#171491
sql: reject volatility weakening when IMMUTABLE-required dependent exists#171491virajchogle wants to merge 2 commits into
Conversation
…ists CREATE OR REPLACE FUNCTION silently accepted weakening a function's volatility while computed columns, expression indexes, or CHECK constraints still required IMMUTABLE, crashing subsequent reads in the optimizer. Reject the replacement in both schema changers and add a nil-check at the virtual-column projection for already-corrupt clusters. Fixes cockroachdb#171486 Epic: none Release note (bug fix): CREATE OR REPLACE FUNCTION now rejects weakening a function's volatility when it is referenced by a computed column,expression index, or CHECK constraint.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
bghal
left a comment
There was a problem hiding this comment.
@bghal made 2 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on spilchen, virajchogle, and ZhouXing19).
pkg/sql/create_function.go line 272 at r1 (raw file):
return err } if err := validateVolatilityForDependents(params.ctx, params.p, udfDesc); err != nil {
Would column defaults going volatile be rejected?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go line 441 at r1 (raw file):
} // updateDependentTriggers finds all triggers that reference the given function
This is hanging.
|
Thanks for looking into just a couple comments. |
…function Move the updateDependentTriggers doc comment so it sits adjacent to its function declaration. The validateVolatilityForDependents function was inserted between the comment and its function, causing godoc to attach the comment to the wrong declaration. Release note: None
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
@bghal Defaults are intentionally out of scope. Column DEFAULTs don't require IMMUTABLE in PG (DEFAULT now(), DEFAULT nextval() are valid). The function runs once at insert/update-with-DEFAULT time and the result is If you have a specific case in mind where a default does require IMMUTABLE, would extend the check to cover it. |
|
Fixed the other hanging issue with another commit, moved the updateDependentTriggers doc comment back next to its function. My bad, completely missed the comment goofup. |
Problem
CREATE OR REPLACE FUNCTIONsilently accepted weakening a function's volatility (e.g.IMMUTABLE→VOLATILE) even when an existing dependent still requiredIMMUTABLE. Reading the dependent rows afterward crashed in the optimizer with a nil-pointer dereference atlogical_props_builder.go:1837.Reproducer:
Fix
Walk the function's dependents at
CREATE OR REPLACE FUNCTIONtime. If any reference uses the function in anIMMUTABLE-required context (computed columns, expression indexes,CHECKconstraints) and the new volatility is anything other thanIMMUTABLE, reject the replacement with an error naming the dependent table.Implemented in both schema changers (legacy and declarative). A defensive nil-check at the optbuilder's virtual-column projection turns the pre-existing nil-deref into a clear error for clusters that already reached the corrupt state.
PG comparison
Tested against PG 17.10 and PG 18.4 in real containers (not just docs):
SELECTreturns the value computed at INSERT time.ERROR: generation expression uses user-defined function. DETAIL: Virtual generated columns that make use of user-defined functions are not yet supported.PG sidesteps this scenario entirely. The error wording in this PR is CRDB-original. (More empirical detail in the issue thread.)
Test coverage
Tests live in
pkg/sql/logictest/testdata/logic_test/udf_in_tableand run across all default logictest configurations (legacy + declarative schema changers, multi-region, mixed-version 25.4/26.1/26.2, etc.). Cases covered:VOLATILE/STABLE/ omitted-volatilityIMMUTABLE(strengthening is fine)VOLATILECHECKconstraint dependent → rejectVOLATILEVOLATILEOut of scope
Views and RLS policies. Their volatility ceilings are looser (
STABLE-or-better) and folding them into the same check would balloon the diff. Can be addressed in a follow-up.Fixes #171486
Epic: none
Release note (bug fix): CREATE OR REPLACE FUNCTION now rejects weakening a function's volatility when it is referenced by a computed column, expression index, or CHECK constraint.