diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 3f0aac09..08c4b3c6 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1918,21 +1918,30 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto } // Separate functions WITHOUT view deps into those that reference deferred - // tables (tablesWithDeps or tablesAfterTableDomains) and those that don't. - // Functions that query new tables must be created after those tables (issue #530). - functionsWithoutTableDeps := functionsWithoutViewDeps - var functionsWithTableDeps []*ir.Function - allDeferredTables := append(tablesWithDeps, tablesAfterTableDomains...) - if len(allDeferredTables) > 0 { - tablesWithDepsLookup := make(map[string]struct{}, len(allDeferredTables)) - for _, table := range allDeferredTables { + // tables and those that don't. Functions that query new tables must be + // created after those tables (issue #530). Functions referencing tables in + // tablesAfterTableDomains (the last table batch) must come after even that + // batch, or a signature using such a table's row type fails to resolve. + buildTableLookup := func(tables []*ir.Table) map[string]struct{} { + lookup := make(map[string]struct{}, len(tables)) + for _, table := range tables { qualified := fmt.Sprintf("%s.%s", strings.ToLower(table.Schema), strings.ToLower(table.Name)) - tablesWithDepsLookup[qualified] = struct{}{} - tablesWithDepsLookup[strings.ToLower(table.Name)] = struct{}{} + lookup[qualified] = struct{}{} + lookup[strings.ToLower(table.Name)] = struct{}{} } + return lookup + } + functionsWithoutTableDeps := functionsWithoutViewDeps + var functionsWithTableDeps []*ir.Function + var functionsAfterAllTables []*ir.Function + if len(tablesWithDeps) > 0 || len(tablesAfterTableDomains) > 0 { + tablesWithDepsLookup := buildTableLookup(tablesWithDeps) + lastBatchLookup := buildTableLookup(tablesAfterTableDomains) functionsWithoutTableDeps = nil for _, fn := range functionsWithoutViewDeps { - if functionReferencesNewTable(fn, tablesWithDepsLookup) { + if functionReferencesNewTable(fn, lastBatchLookup) { + functionsAfterAllTables = append(functionsAfterAllTables, fn) + } else if functionReferencesNewTable(fn, tablesWithDepsLookup) { functionsWithTableDeps = append(functionsWithTableDeps, fn) } else { functionsWithoutTableDeps = append(functionsWithoutTableDeps, fn) @@ -1967,6 +1976,9 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto // Create tables that use table-dep domains as column types (now that those domains exist). deferredPolicies3, deferredConstraints3 := generateCreateTablesSQL(tablesAfterTableDomains, targetSchema, collector, existingTables, shouldDeferPolicy, d.suppressedInlineFKs, d.allNewTables) + // Create functions that reference tables in the last table batch above. + generateCreateFunctionsSQL(functionsAfterAllTables, targetSchema, collector) + // Emit COMMENT ON SEQUENCE for sequences created implicitly via CREATE TABLE (SERIAL/BIGSERIAL). // These were skipped from addedSequences but their comments must still be deployed. for _, seq := range d.addedSerialSeqComments { @@ -2469,14 +2481,31 @@ func buildRecreatedViewLookup(modifiedViews []*viewDiff) map[string]struct{} { // in its return type or parameter types. This handles cases where functions use // view composite types (e.g., RETURNS SETOF view_name or parameter of view_name type). func functionReferencesNewView(fn *ir.Function, newViews map[string]struct{}) bool { - if len(newViews) == 0 || fn == nil { + return functionSignatureReferencesRelation(fn, newViews) +} + +// functionSignatureReferencesRelation determines if a function's return type or +// parameter types reference a relation in the lookup. PostgreSQL exposes both +// tables and views as composite types, so a function using one in its signature +// must be created after that relation exists. +func functionSignatureReferencesRelation(fn *ir.Function, relations map[string]struct{}) bool { + if len(relations) == 0 || fn == nil { return false } // Check return type (e.g., "SETOF public.actor", "actor", "SETOF actor") if fn.ReturnType != "" { typeName := extractBaseTypeName(fn.ReturnType) - if typeMatchesLookup(typeName, fn.Schema, newViews) { + if typeMatchesLookup(typeName, fn.Schema, relations) { + return true + } + } + + // Check output column types of a TABLE(...) return type (e.g., + // "TABLE(r uses, n integer)"). pg_get_function_arguments excludes + // TABLE-mode columns, so they only appear in the return type. + for _, colType := range tableReturnColumnTypes(fn.ReturnType) { + if typeMatchesLookup(extractBaseTypeName(colType), fn.Schema, relations) { return true } } @@ -2485,7 +2514,7 @@ func functionReferencesNewView(fn *ir.Function, newViews map[string]struct{}) bo for _, param := range fn.Parameters { if param.DataType != "" { typeName := extractBaseTypeName(param.DataType) - if typeMatchesLookup(typeName, fn.Schema, newViews) { + if typeMatchesLookup(typeName, fn.Schema, relations) { return true } } @@ -2494,6 +2523,61 @@ func functionReferencesNewView(fn *ir.Function, newViews map[string]struct{}) bo return false } +// tableReturnColumnTypes extracts the column type expressions from a TABLE(...) +// return type produced by pg_get_function_result, e.g. "TABLE(r uses, n integer)" +// yields ["uses", "integer"]. Returns nil when the return type is not TABLE(...). +func tableReturnColumnTypes(returnType string) []string { + t := strings.TrimSpace(returnType) + if len(t) < 7 || !strings.EqualFold(t[:6], "TABLE(") || !strings.HasSuffix(t, ")") { + return nil + } + inner := t[6 : len(t)-1] + + var types []string + appendColType := func(col string) { + col = strings.TrimSpace(col) + // Strip the leading column name (possibly a quoted identifier + // containing spaces) to leave the type expression. + var typeExpr string + if strings.HasPrefix(col, `"`) { + if end := strings.Index(col[1:], `"`); end >= 0 { + typeExpr = col[end+2:] + } + } else if idx := strings.IndexByte(col, ' '); idx >= 0 { + typeExpr = col[idx+1:] + } + if typeExpr = strings.TrimSpace(typeExpr); typeExpr != "" { + types = append(types, typeExpr) + } + } + + // Split on top-level commas, ignoring commas inside parentheses (e.g. + // numeric(10,2)) and quoted identifiers. + depth, start := 0, 0 + inQuote := false + for i := 0; i < len(inner); i++ { + switch inner[i] { + case '"': + inQuote = !inQuote + case '(': + if !inQuote { + depth++ + } + case ')': + if !inQuote { + depth-- + } + case ',': + if !inQuote && depth == 0 { + appendColType(inner[start:i]) + start = i + 1 + } + } + } + appendColType(inner[start:]) + return types +} + // extractBaseTypeName extracts the base type name from a type expression, // stripping SETOF prefix, array notation, and double quotes from identifiers. func extractBaseTypeName(typeExpr string) string { @@ -2717,14 +2801,25 @@ var tableRefPattern = regexp.MustCompile( `([a-z_][a-z0-9_$]*(?:\.[a-z_][a-z0-9_$]*)*)`, ) -// functionReferencesNewTable determines if a function body references any newly +// functionReferencesNewTable determines if a function references any newly // added table that will be created after the first function batch (tablesWithDeps). -// It looks for table names in SQL table-reference contexts (FROM, JOIN, -// INSERT INTO, UPDATE, DELETE FROM) rather than scanning the entire body, -// avoiding false positives from comments, literals, and aliases. +// It checks the function signature for table composite types (issue #545) and +// looks for table names in SQL table-reference contexts (FROM, JOIN, +// INSERT INTO, UPDATE, DELETE FROM) in the body rather than scanning the entire +// body, avoiding false positives from comments, literals, and aliases. // See https://github.com/pgplex/pgschema/issues/530 func functionReferencesNewTable(fn *ir.Function, newTables map[string]struct{}) bool { - if len(newTables) == 0 || fn == nil || fn.Definition == "" { + if len(newTables) == 0 || fn == nil { + return false + } + + // A table also defines an implicit composite row type, so a function using + // it as a parameter or return type must be created after the table. + if functionSignatureReferencesRelation(fn, newTables) { + return true + } + + if fn.Definition == "" { return false } diff --git a/testdata/diff/dependency/issue_530_function_table_function_chain/diff.sql b/testdata/diff/dependency/issue_530_function_table_function_chain/diff.sql index 88773336..3c93c252 100644 --- a/testdata/diff/dependency/issue_530_function_table_function_chain/diff.sql +++ b/testdata/diff/dependency/issue_530_function_table_function_chain/diff.sql @@ -16,6 +16,18 @@ CREATE TABLE IF NOT EXISTS x ( CONSTRAINT x_public_id_key UNIQUE (public_id) ); +CREATE OR REPLACE FUNCTION x_check( + row_x x +) +RETURNS boolean +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN row_x.flag; +END; +$$; + CREATE OR REPLACE FUNCTION x_is_flagged( id bigint ) diff --git a/testdata/diff/dependency/issue_530_function_table_function_chain/new.sql b/testdata/diff/dependency/issue_530_function_table_function_chain/new.sql index f3af9f1d..19bee107 100644 --- a/testdata/diff/dependency/issue_530_function_table_function_chain/new.sql +++ b/testdata/diff/dependency/issue_530_function_table_function_chain/new.sql @@ -14,3 +14,12 @@ CREATE FUNCTION public.x_is_flagged(id bigint) RETURNS boolean LANGUAGE sql STABLE AS $$ SELECT x.flag FROM x WHERE x.id = id; $$; + +-- Issue #545: depends on table x only through its parameter type (implicit +-- composite row type); the body never references the table itself. +CREATE FUNCTION public.x_check(row_x x) +RETURNS boolean LANGUAGE plpgsql AS $$ +BEGIN + RETURN row_x.flag; +END; +$$; diff --git a/testdata/diff/dependency/issue_530_function_table_function_chain/plan.json b/testdata/diff/dependency/issue_530_function_table_function_chain/plan.json index 60da7fa9..1d840f35 100644 --- a/testdata/diff/dependency/issue_530_function_table_function_chain/plan.json +++ b/testdata/diff/dependency/issue_530_function_table_function_chain/plan.json @@ -1,6 +1,6 @@ { "version": "1.0.0", - "pgschema_version": "1.12.2", + "pgschema_version": "1.12.3", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { "hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085" @@ -20,6 +20,12 @@ "operation": "create", "path": "public.x" }, + { + "sql": "CREATE OR REPLACE FUNCTION x_check(\n row_x x\n)\nRETURNS boolean\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n RETURN row_x.flag;\nEND;\n$$;", + "type": "function", + "operation": "create", + "path": "public.x_check" + }, { "sql": "CREATE OR REPLACE FUNCTION x_is_flagged(\n id bigint\n)\nRETURNS boolean\nLANGUAGE sql\nSTABLE\nAS $$\n SELECT x.flag FROM x WHERE x.id = id;\n$$;", "type": "function", diff --git a/testdata/diff/dependency/issue_530_function_table_function_chain/plan.sql b/testdata/diff/dependency/issue_530_function_table_function_chain/plan.sql index 88773336..3c93c252 100644 --- a/testdata/diff/dependency/issue_530_function_table_function_chain/plan.sql +++ b/testdata/diff/dependency/issue_530_function_table_function_chain/plan.sql @@ -16,6 +16,18 @@ CREATE TABLE IF NOT EXISTS x ( CONSTRAINT x_public_id_key UNIQUE (public_id) ); +CREATE OR REPLACE FUNCTION x_check( + row_x x +) +RETURNS boolean +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN row_x.flag; +END; +$$; + CREATE OR REPLACE FUNCTION x_is_flagged( id bigint ) diff --git a/testdata/diff/dependency/issue_530_function_table_function_chain/plan.txt b/testdata/diff/dependency/issue_530_function_table_function_chain/plan.txt index f011015e..8643816c 100644 --- a/testdata/diff/dependency/issue_530_function_table_function_chain/plan.txt +++ b/testdata/diff/dependency/issue_530_function_table_function_chain/plan.txt @@ -1,11 +1,12 @@ -Plan: 3 to add. +Plan: 4 to add. Summary by type: - functions: 2 to add + functions: 3 to add tables: 1 to add Functions: + random_id + + x_check + x_is_flagged Tables: @@ -32,6 +33,18 @@ CREATE TABLE IF NOT EXISTS x ( CONSTRAINT x_public_id_key UNIQUE (public_id) ); +CREATE OR REPLACE FUNCTION x_check( + row_x x +) +RETURNS boolean +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN row_x.flag; +END; +$$; + CREATE OR REPLACE FUNCTION x_is_flagged( id bigint ) diff --git a/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/diff.sql b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/diff.sql new file mode 100644 index 00000000..28a0dd89 --- /dev/null +++ b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/diff.sql @@ -0,0 +1,31 @@ +CREATE TABLE IF NOT EXISTS base ( + +); + +CREATE DOMAIN d AS base; + +CREATE TABLE IF NOT EXISTS uses ( + dcol d +); + +CREATE OR REPLACE FUNCTION uses_check( + r uses +) +RETURNS boolean +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN r.dcol IS NOT NULL; +END; +$$; + +CREATE OR REPLACE FUNCTION uses_rows() +RETURNS TABLE(r uses) +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN; +END; +$$; diff --git a/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/new.sql b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/new.sql new file mode 100644 index 00000000..99c20724 --- /dev/null +++ b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/new.sql @@ -0,0 +1,27 @@ +CREATE TABLE base (); + +CREATE DOMAIN d AS base; + +CREATE TABLE uses ( + dcol d +); + +-- Function signature references table "uses", which itself must be created +-- after domain d (whose base type is table base's row type). The function +-- must therefore come after the last table batch. +CREATE FUNCTION public.uses_check(r uses) +RETURNS boolean LANGUAGE plpgsql AS $$ +BEGIN + RETURN r.dcol IS NOT NULL; +END; +$$; + +-- Depends on table "uses" only through the TABLE(...) output column type; +-- these columns are excluded from pg_get_function_arguments and only appear +-- in pg_get_function_result as "TABLE(r uses)". +CREATE FUNCTION public.uses_rows() +RETURNS TABLE(r uses) LANGUAGE plpgsql AS $$ +BEGIN + RETURN; +END; +$$; diff --git a/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/old.sql b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/old.sql new file mode 100644 index 00000000..e69de29b diff --git a/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.json b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.json new file mode 100644 index 00000000..39b553c6 --- /dev/null +++ b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.json @@ -0,0 +1,44 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.12.3", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS base (\n\n);", + "type": "table", + "operation": "create", + "path": "public.base" + }, + { + "sql": "CREATE DOMAIN d AS base;", + "type": "domain", + "operation": "create", + "path": "public.d" + }, + { + "sql": "CREATE TABLE IF NOT EXISTS uses (\n dcol d\n);", + "type": "table", + "operation": "create", + "path": "public.uses" + }, + { + "sql": "CREATE OR REPLACE FUNCTION uses_check(\n r uses\n)\nRETURNS boolean\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n RETURN r.dcol IS NOT NULL;\nEND;\n$$;", + "type": "function", + "operation": "create", + "path": "public.uses_check" + }, + { + "sql": "CREATE OR REPLACE FUNCTION uses_rows()\nRETURNS TABLE(r uses)\nLANGUAGE plpgsql\nVOLATILE\nAS $$\nBEGIN\n RETURN;\nEND;\n$$;", + "type": "function", + "operation": "create", + "path": "public.uses_rows" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.sql b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.sql new file mode 100644 index 00000000..28a0dd89 --- /dev/null +++ b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.sql @@ -0,0 +1,31 @@ +CREATE TABLE IF NOT EXISTS base ( + +); + +CREATE DOMAIN d AS base; + +CREATE TABLE IF NOT EXISTS uses ( + dcol d +); + +CREATE OR REPLACE FUNCTION uses_check( + r uses +) +RETURNS boolean +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN r.dcol IS NOT NULL; +END; +$$; + +CREATE OR REPLACE FUNCTION uses_rows() +RETURNS TABLE(r uses) +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN; +END; +$$; diff --git a/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.txt b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.txt new file mode 100644 index 00000000..9564814e --- /dev/null +++ b/testdata/diff/dependency/issue_545_signature_ref_table_after_domain/plan.txt @@ -0,0 +1,48 @@ +Plan: 5 to add. + +Summary by type: + functions: 2 to add + tables: 2 to add + +Functions: + + uses_check + + uses_rows + +Tables: + + base + + uses + +DDL to be executed: +-------------------------------------------------- + +CREATE TABLE IF NOT EXISTS base ( + +); + +CREATE DOMAIN d AS base; + +CREATE TABLE IF NOT EXISTS uses ( + dcol d +); + +CREATE OR REPLACE FUNCTION uses_check( + r uses +) +RETURNS boolean +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN r.dcol IS NOT NULL; +END; +$$; + +CREATE OR REPLACE FUNCTION uses_rows() +RETURNS TABLE(r uses) +LANGUAGE plpgsql +VOLATILE +AS $$ +BEGIN + RETURN; +END; +$$;