fix: function dependencies #258
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances dependency handling so that function creation order respects references that appear inside SQL function bodies, fixing cases where wrappers call helpers that would otherwise be created later alphabetically.
Changes:
- Add
buildFunctionBodyDependencies()ininternal/diff/topological.goto scan function bodies usingfunctionCallRegexand populateir.Function.Dependenciesbased on detected function calls. - Wire body-based dependency detection into function diff generation (
generateCreateFunctionsSQL) and extend dependency-based ordering inGenerateMigrationso that functions and tables are created in a safe order relative to each other. - Introduce new test fixtures and tests (including integration tests and updated sakila dump) that validate correct function ordering when dependencies exist only in SQL function bodies.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/diff/topological.go |
Adds buildFunctionBodyDependencies() and integrates it with existing topological sort logic for functions using the shared functionCallRegex. |
internal/diff/function.go |
Calls buildFunctionBodyDependencies() before topologicallySortFunctions() to ensure CREATE FUNCTION statements are emitted in dependency order based on function bodies. |
internal/diff/diff.go |
Introduces buildFunctionLookup, functionCallRegex, and helpers to detect when tables/policies reference newly added functions; refactors ddlDiff struct and initializer for clarity. |
internal/diff/topological_test.go |
Adds unit tests for buildFunctionBodyDependencies() and an integration test that verifies body-derived dependencies influence topologicallySortFunctions() as expected. |
testdata/diff/dependency/sql_function_body_reference/* |
New file-based diff test fixtures that reproduce a wrapper/helper SQL-function scenario where body-only dependencies must drive function creation order. |
testdata/dump/sakila/pgschema.sql |
Updates the dump header (PostgreSQL 18.0 / pgschema 1.6.1) and repositions film_in_stock/film_not_in_stock functions to reflect the new dependency-aware ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+226
to
+365
| func TestBuildFunctionBodyDependencies(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| funcs []*ir.Function | ||
| expected map[string][]string // function name -> expected dependency names | ||
| }{ | ||
| { | ||
| name: "simple dependency: wrapper calls helper", | ||
| funcs: []*ir.Function{ | ||
| { | ||
| Schema: "public", | ||
| Name: "wrapper", | ||
| Definition: "SELECT helper()", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "helper", | ||
| Definition: "SELECT 1", | ||
| Language: "sql", | ||
| }, | ||
| }, | ||
| expected: map[string][]string{ | ||
| "wrapper": {"public.helper()"}, | ||
| "helper": nil, | ||
| }, | ||
| }, | ||
| { | ||
| name: "qualified function call", | ||
| funcs: []*ir.Function{ | ||
| { | ||
| Schema: "public", | ||
| Name: "caller", | ||
| Definition: "SELECT public.callee()", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "callee", | ||
| Definition: "SELECT 1", | ||
| Language: "sql", | ||
| }, | ||
| }, | ||
| expected: map[string][]string{ | ||
| "caller": {"public.callee()"}, | ||
| "callee": nil, | ||
| }, | ||
| }, | ||
| { | ||
| name: "chain: a calls b calls c", | ||
| funcs: []*ir.Function{ | ||
| { | ||
| Schema: "public", | ||
| Name: "func_a", | ||
| Definition: "SELECT func_b()", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "func_b", | ||
| Definition: "SELECT func_c()", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "func_c", | ||
| Definition: "SELECT 1", | ||
| Language: "sql", | ||
| }, | ||
| }, | ||
| expected: map[string][]string{ | ||
| "func_a": {"public.func_b()"}, | ||
| "func_b": {"public.func_c()"}, | ||
| "func_c": nil, | ||
| }, | ||
| }, | ||
| { | ||
| name: "no self-dependency", | ||
| funcs: []*ir.Function{ | ||
| { | ||
| Schema: "public", | ||
| Name: "recursive", | ||
| Definition: "SELECT recursive()", // calls itself | ||
| Language: "sql", | ||
| }, | ||
| }, | ||
| expected: map[string][]string{ | ||
| "recursive": nil, // should not add self as dependency | ||
| }, | ||
| }, | ||
| { | ||
| name: "multiple calls in body", | ||
| funcs: []*ir.Function{ | ||
| { | ||
| Schema: "public", | ||
| Name: "orchestrator", | ||
| Definition: "SELECT step_one() + step_two() + step_three()", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "step_one", | ||
| Definition: "SELECT 1", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "step_two", | ||
| Definition: "SELECT 2", | ||
| Language: "sql", | ||
| }, | ||
| { | ||
| Schema: "public", | ||
| Name: "step_three", | ||
| Definition: "SELECT 3", | ||
| Language: "sql", | ||
| }, | ||
| }, | ||
| expected: map[string][]string{ | ||
| "orchestrator": {"public.step_one()", "public.step_two()", "public.step_three()"}, | ||
| "step_one": nil, | ||
| "step_two": nil, | ||
| "step_three": nil, | ||
| }, | ||
| }, | ||
| { | ||
| name: "external function not tracked", | ||
| funcs: []*ir.Function{ | ||
| { | ||
| Schema: "public", | ||
| Name: "my_func", | ||
| Definition: "SELECT pg_catalog.now() + external_func()", | ||
| Language: "sql", | ||
| }, | ||
| }, | ||
| expected: map[string][]string{ | ||
| "my_func": nil, // external functions not in our list | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
These tests cover SQL-language functions but not the plpgsql function-body dependency scenario described in issue #256. To guard against regressions in that specific case, consider adding a file-based diff test under testdata/diff/dependency/ (or an additional unit test here) where a newly added plpgsql wrapper function calls another newly added function, and assert that the generated plan orders them correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #256
split out from #255
Problem
PostgreSQL's
pg_dependcatalog doesn't track function-to-function references inside SQL function bodies, so the existing topological sort had no dependency data to work with and fell back to alphabetical ordering. This caused failures when a function likea_wrapper()calledz_helper()— alphabeticallya_wrapperwould be created first, but it needsz_helperto exist.Fix
Added
buildFunctionBodyDependencies()which scans function bodies using the existingfunctionCallRegexto detect function calls, then populates theDependenciesfield before the topological sort runs. This supplements thepg_dependdata with body-level references.