Repository navigation
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes GitHub issue #125 where function parameter default values were being lost when dumping functions from the database. The fix simplifies parameter parsing by relying solely on pg_get_function_arguments() which already includes complete parameter information including defaults, rather than using a dual-path approach with PostgreSQL system catalog arrays.
Key Changes:
- Removed the
parseParametersFromProcArraysmethod and its associated system catalog queries - Simplified parameter parsing to use only
parseParametersFromSignaturewhich processes the complete signature frompg_get_function_arguments() - Added comprehensive test coverage with 5 test cases covering simple defaults, mixed IN/OUT parameters, INOUT parameters, complex default expressions, and VARIADIC parameters
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/dump/issue_125_function_default/raw.sql | Adds test cases with functions using various parameter default patterns |
| testdata/dump/issue_125_function_default/pgschema.sql | Expected output showing preserved default values in dumped functions |
| testdata/dump/issue_125_function_default/pgdump.sql | Reference pg_dump output for comparison |
| testdata/dump/issue_125_function_default/manifest.json | Test metadata describing the issue being tested |
| ir/queries/queries.sql | Removes unused system catalog array fields from function query |
| ir/inspector.go | Removes dual-path parameter parsing, uses only signature-based parsing |
| cmd/dump/dump_integration_test.go | Adds integration test for issue #125 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| CREATE OR REPLACE FUNCTION test_mixed_params( | ||
| raise_on_error boolean DEFAULT true, | ||
| p_schema_override text DEFAULT NULL, |
There was a problem hiding this comment.
The default value DEFAULT NULL differs from the input in raw.sql which specifies DEFAULT NULL::text. The explicit cast to text should be preserved to match PostgreSQL's behavior and pg_dump's output (line 36 in pgdump.sql shows DEFAULT NULL::text).
| p_schema_override text DEFAULT NULL, | |
| p_schema_override text DEFAULT NULL::text, |
| CREATE OR REPLACE FUNCTION test_inout_params( | ||
| value integer DEFAULT 100, | ||
| INOUT multiplier numeric DEFAULT 1.5, | ||
| INOUT result numeric DEFAULT NULL |
There was a problem hiding this comment.
The default value DEFAULT NULL is missing the explicit type cast. According to pgdump.sql line 49, this should be DEFAULT NULL::numeric to properly specify the typed null value.
| INOUT result numeric DEFAULT NULL | |
| INOUT result numeric DEFAULT NULL::numeric |
Fix #125