fix: order aggregates and SQL function bodies after the views they depend on (#580)

[tianzhou]

Summary

Follow-up to #581, covering the two remaining cases from #580 that live in the diff package rather than the multi-file formatter. Both reproduced in single-file dump and therefore in plan/apply against an empty database.

• Aggregate over a view row type. An aggregate whose argument, state, or return type names a new view was emitted before the view, and before its own transition function when that function takes the row type. Aggregates with a view dependency are now partitioned out and created after the view-dependent functions, with the same deferred-view handling (issue Bug: Views and New column create are not ordered correctly #414 path) that functions already have.
• SQL-language function body querying a view. functionReferencesNewView was signature-only. It now also scans the body for SQL-language functions, reusing the existing tableRefPattern through a shared bodyReferencesRelation helper that functionReferencesNewTable uses as well. Other languages stay signature-only on purpose: plpgsql bodies are not validated against relations at creation, and triggers are created before views, so treating body mentions as dependencies would push trigger functions past the triggers that call them. The test case includes exactly that plpgsql trigger function to pin the behavior.

Fixing the aggregate order exposed a pre-existing round-trip bug that the apply harness caught: aggregate type strings were never stripped of the aggregate's own schema prefix, so an aggregate over a relation row type inspected as sum_v(public.v) on one side and sum_v(v) on the other and was dropped and recreated on every plan. buildAggregates now strips the prefix from the search_path-dependent fields Arguments, Signature, and ReturnType, mirroring what function parameters do via stripSameSchemaPrefix. State types are left as the catalog query qualifies them, since the diff layer strips or keeps that prefix for --qualify-schema.

Fixes #580

Test plan

• New testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view covering a SQL function querying a view, a plpgsql trigger function mentioning a view, and an aggregate over the view row type with a row-type transition function. Fails before the fix with relation "v" does not exist; after the ordering fix it then failed the idempotency check until the inspector fix; now passes.
• TestDumpCommand_Issue580MultiFileIncludeOrder extended with both scenarios and asserts view before function, view before transition function, transition function before aggregate.
• Passing locally: full TestDiffFromFiles, TestPlanAndApply for dependency/, create_aggregate/, create_function/, and TestDumpCommand_Sakila.

PGSCHEMA_TEST_FILTER="dependency/issue_580_aggregate_and_sql_body_to_view" go test -v ./cmd -run TestPlanAndApply

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR extends dependency ordering for aggregates and SQL function bodies that reference newly created views, adds deferred aggregate handling, and normalizes same-schema aggregate type names during inspection. The new ordering still has two uncovered dependency cases:

• An aggregate can precede a view-dependent support function when the aggregate’s own types do not reference the view.
• Lexical matches inside SQL comments or literals can falsely defer a function behind a view that calls it.

Confidence Score: 3/5

This PR is not safe to merge until aggregate support-function dependencies and false-positive SQL body matches can no longer produce invalid creation order.

Two realistic schema definitions still generate migrations in which PostgreSQL attempts to create a dependent object before the required function exists.

Files Needing Attention: internal/diff/diff.go

Important Files Changed

Filename	Overview
internal/diff/diff.go	Adds view-body dependency detection and deferred aggregate ordering, but leaves two migration-failing ordering paths.
ir/inspector.go	Canonicalizes same-schema aggregate argument, signature, return, and state type representations.
cmd/dump/multifile_integration_test.go	Covers direct aggregate row-type dependencies but not view-dependent support functions with ordinary aggregate types or lexical false positives.
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/new.sql	Adds an end-to-end fixture for SQL functions and aggregate row types that depend directly on a new view.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  V[New view] --> VF[SQL functions classified as view-dependent]
  VF --> VA[Aggregates whose own types reference the view]
  A[Aggregate with ordinary types] --> E[Early aggregate batch]
  V -. queried by support-function body .-> SF[Aggregate support function]
  SF --> VF
  E --> X[CREATE AGGREGATE before support function exists]
  L[Comment or string containing FROM view] --> FP[False view dependency]
  FP --> VF
  VC[View calling the function] --> Y[CREATE VIEW before function exists]

Loading

_{Reviews (1): Last reviewed commit: "fix: order aggregates and SQL function b..." | Re-trigger Greptile}

[Copilot]

🟡 Changes recommended

There are correctness issues in the new aggregate identity normalization and in aggregate/view dependency handling that can still cause plan churn or ordering failures in valid schemas.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses dependency-ordering bugs in the diff engine so plan/apply and single-file dumps don’t emit view-dependent objects (SQL-language function bodies and aggregates over a view row type) before the views they require, and adds regression coverage for issue #580.

Changes:

• Extend functionReferencesNewView to treat SQL-language function bodies as creation-time dependencies on referenced views (while keeping other languages signature-only).
• Partition/defer aggregates whose argument/state/return types reference newly-added (or deferred) views so they are created after the required views and view-dependent functions.
• Normalize inspected aggregate type/identity strings to reduce schema-qualification churn and improve idempotency.

File summaries

File	Description
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/plan.txt	Adds expected human-readable plan output for the new dependency regression.
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/plan.sql	Adds expected SQL plan output for the new dependency regression.
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/plan.json	Adds expected JSON plan output for the new dependency regression.
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/old.sql	Empty baseline for the new diff test case.
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/new.sql	New schema input reproducing SQL-body-to-view and aggregate-over-view-row-type cases.
testdata/diff/dependency/issue_580_aggregate_and_sql_body_to_view/diff.sql	Expected diff ordering for the new test case.
ir/inspector.go	Attempts to normalize aggregate identity/signature/type strings by stripping same-schema prefixes.
internal/diff/diff.go	Implements the new deferral/partitioning logic for view-dependent aggregates and SQL-body view detection.
cmd/dump/multifile_integration_test.go	Extends multi-file dump integration assertions to cover the issue #580 follow-up scenarios.

Review details

• Files reviewed: 8/9 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Three critical dependency-ordering issues and one moderate schema-normalization issue remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 8/9 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Five moderate dependency-ordering and reference-parsing issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

internal/diff/diff.go:2062

• Aggregates already placed in aggregatesWithViewDeps are never checked against recreatedViewLookup. An aggregate that depends on both a newly added view and a recreated view (or on a support function deferred for that recreated view) therefore stays in the create phase; it can be emitted before its support function exists or pin the old view and make the later DROP ... RESTRICT fail. Partition both aggregate buckets against recreated-view dependencies before emission.

	aggregatesToCreateNow, d.aggregatesAwaitingRecreatedViews = splitAggregatesByViewDeps(aggregatesToCreateNow, recreatedViewLookup, buildFunctionLookup(d.functionsAwaitingRecreatedViews))

internal/diff/diff.go:2202

• These routines are emitted only after all added views were handled in the create phase, but the create-phase routine lookup excludes functionsAwaitingRecreatedViews and aggregatesAwaitingRecreatedViews. Thus a new view that calls one of these deferred routines is created while that routine is absent (for example, a new view calling an aggregate whose SFUNC is typed on a recreated view), and CREATE VIEW fails. Move such dependent added views to this phase and schedule them with the queued routines.

	generateCreateFunctionsSQL(d.functionsAwaitingRecreatedViews, targetSchema, collector)
	generateCreateAggregatesSQL(d.aggregatesAwaitingRecreatedViews, targetSchema, collector)

• Files reviewed: 19/20 changed files
• Comments generated: 3
• Review effort level: Balanced

[tianzhou]

Re the two suppressed comments in the latest review, both addressed in the last commit:

• Aggregates in aggregatesWithViewDeps never checked against recreated views. The recreated-view partition now runs first over all added aggregates, so an aggregate that depends on both a new view and a recreated view (or a support function held for one) waits for the recreation. issue_480 fixture gains agg_pair(vw_users, vw_active) with a transition function typed on both; it is emitted after fn_pair_step in the modify phase.
• New views calling routines held for a recreation. Such views are now split out in the create phase into viewsAwaitingRecreatedViews and scheduled in the modify phase with those routines through the same generateViewsAndDependentRoutinesSQL helper. issue_480 fixture gains vw_users_count calling agg_users_count; it is emitted last, after the aggregate.

[Copilot]

🟡 Changes recommended

A critical function-to-aggregate ordering failure remains unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 19/20 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate dependency-ordering and normalization issues can produce invalid or non-idempotent plans.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

internal/diff/diff.go:2988

• This substring search also finds ORDER BY inside a quoted argument name or type. For a valid view row type such as "Order By V", the identity argument is split into invalid fragments, so the aggregate's view dependency is missed and it is emitted before the view. Locate the ordered-set separator only outside quoted identifiers and at token boundaries.
  ir/inspector.go:1433
• Leaving state types qualified still causes non-idempotent plans for target schemas that require quoting. The desired temporary schema's temp.state_type is remapped to MySchema.state_type, while current inspection returns "MySchema".state_type; aggregatesEqual compares these strings before DDL stripping, so the aggregate is recreated every plan. Normalize StateType and MStateType consistently (and preserve --qualify-schema behavior), as the PR description states.

internal/diff/diff.go:2076

• functionsCallingAggregates was selected against all added aggregates, but this emits it after only aggregatesToCreateNow. If a SQL function calls an aggregate deferred for a new/recreated view without mentioning that view itself (for example, a caller of sum_with_v(integer)), PostgreSQL validates the caller here before the aggregate exists. Partition callers by the aggregate batch and emit each caller with its aggregate instead.

	// SQL-language functions that call one of the aggregates above.
	generateCreateFunctionsSQL(functionsCallingAggregates, targetSchema, collector)

• Files reviewed: 24/25 changed files
• Comments generated: 3
• Review effort level: Balanced

[tianzhou]

Re the three suppressed comments in the latest review:

• Aggregate callers emitted only after aggregatesToCreateNow. Fixed: callers are now split by which batch creates their aggregate. Those calling a view-held aggregate join functionsWithViewDeps, those calling a recreated-batch aggregate join functionsAwaitingRecreatedViews, and the shared helper places each after its aggregate. issue_580 gains total_with_v(), which calls sum_with_v without mentioning v, and lands after the aggregate.
• ORDER BY found inside a quoted identifier. Fixed: the separator is now located only outside quoted identifiers and at a token boundary. issue_580 gains a view named "Order By V" with an aggregate over its row type.
• State types for a target schema that needs quoting. Confirmed as a real gap, but pre-existing and untouched by this PR: newSchemaStringReplacer maps the temp schema prefix to the unquoted target name while the inspector emits the quote_ident form, so aggregatesEqual sees MySchema.acc vs "MySchema".acc. This PR deliberately leaves state types as the query produces them so --qualify-schema keeps working (see the earlier CI failure). The fix belongs in the plan-side replacer or in the comparison, affects every field passing through it, and deserves its own change with a quoted-schema plan test. I've corrected the PR description, which still claimed state types were stripped.

[Copilot]

🟡 Changes recommended

Three critical defects and one moderate ordering defect remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/diff/diff.go:1984

• This only checks functionsWithoutViewDeps. A SQL function already classified as view-dependent can call an aggregate queued for the recreated-view phase (for example, the aggregate's support function depends on a recreated view); that function remains in the create-phase view batch and is emitted before the aggregate exists. Partition callers across every function bucket, then attach each caller to the batch that creates its referenced aggregate.

	functionsWithoutViewDeps, functionsCallingAggregates = splitFunctionsCallingAggregates(functionsWithoutViewDeps, d.addedAggregates)

• Files reviewed: 24/25 changed files
• Comments generated: 3
• Review effort level: Balanced

[tianzhou]

Re the suppressed comment (view-dependent SQL function calling an aggregate queued for the recreated-view batch): fixed in the last commit. After callers are routed by aggregate batch, functionsWithViewDeps is also split against aggregatesAwaitingRecreatedViews, and matches move to functionsAwaitingRecreatedViews. Folded into issue_480: active_marker(a vw_active) calls agg_users_count without naming the recreated view and is now emitted in the modify phase after the aggregate.

[Copilot]

🟡 Changes recommended

Critical dependency-scheduling defects and moderate SQL parsing gaps can still produce invalid creation order.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 26/27 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Four critical dependency detection and ordering defects remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 27/28 changed files
• Comments generated: 4
• Review effort level: Balanced