Skip to content

fix: name-typed columns dumped as char[]#411

Merged
tianzhou merged 3 commits intopgplex:mainfrom
ivgiuliani:char-bug
Apr 27, 2026
Merged

fix: name-typed columns dumped as char[]#411
tianzhou merged 3 commits intopgplex:mainfrom
ivgiuliani:char-bug

Conversation

@ivgiuliani
Copy link
Copy Markdown
Contributor

Columns declared as name are incorrectly dumped as char[] because the inspector classifies any base type with pg_type.typelem <> 0 as an array. This changes it to use typcategory = 'A' instead which is a more narrow type check (docs).

Fixes #410

Running sqlc locally for me seems to have a slightly different output, I suspect this is because previous edits to the file have been done by LLMs rather than by running sqlc itself? I kept that output in a separate commit for clarity.

Adds a failing integration test confirming the inspector misclassifies
columns declared as `name` as `char[]`, because pg_type.typelem is
non-zero for `name` (points to char) even though `name` is not an array.
Picks up two regenerator-driven changes that had drifted from the
checked-in output:

- IsPeriod is now sql.NullBool because sqlc no longer infers non-null
  from COALESCE((... ->> 'conperiod')::boolean, false). Updated
  inspector.go to use .Bool when reading the field.
- GetFunctionDependencies is emitted in source-file order (between
  GetEnumValuesForSchema and GetFunctions) rather than at the end.

No behavior changes.
The inspector treated any base type with pg_type.typelem <> 0 as an
array. The `name` type is a non-array fixed-length type whose typelem
points to char (OID 18), so it was misclassified and dumped as `char[]`.

Switch the predicate to typcategory = 'A', which is PostgreSQL's
intended way to identify array types.
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 27, 2026

Greptile Summary

This PR fixes a bug where columns declared as name were incorrectly dumped as char[] by changing the array-detection predicate in two SQL queries from dt.typelem <> 0 to dt.typcategory = 'A' — the canonical PostgreSQL way to identify array types. The queries.sql.go file is re-generated with sqlc (moving GetFunctionDependencies and updating IsPeriod to sql.NullBool), and a new integration test directly covers the regression.

Confidence Score: 5/5

Safe to merge — the bug fix is correct and the sole P2 note is cosmetic.

The core change (typcategory = 'A') is the documented PostgreSQL approach for array identification and is applied consistently across all four query occurrences. The generated Go file matches sqlc's output, the inspector update is trivially correct, and the new integration test directly guards the regression. The only observation is a cosmetic sql.NullBool vs bool discrepancy that carries no runtime risk.

No files require special attention.

Important Files Changed

Filename Overview
ir/queries/queries.sql Replaces typelem <> 0 with typcategory = 'A' in the CASE expression for array detection — the targeted, correct fix for misclassifying name as char[].
ir/queries/queries.sql.go sqlc-regenerated file: mirrors SQL fix in two query constants, moves GetFunctionDependencies earlier, and changes IsPeriod from bool to sql.NullBool — the NullBool is consistent with sqlc's output but diverges from the COALESCE in the SQL source.
ir/inspector.go Single-line update to use .Bool on the now-NullBool IsPeriod field; functionally correct since COALESCE in SQL prevents NULL.
cmd/dump/dump_integration_test.go Adds a focused integration test that creates a name-typed column and asserts the dump output contains n name and not char[], directly covering the regression.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Column type introspection] --> B{dt.typtype = 'b'?}
    B -- No --> C[Handle domain / enum / composite / fallback]
    B -- Yes --> D{"OLD: typelem <> 0\nNEW: typcategory = 'A'"}
    D -- "OLD: true for name\nNEW: false for name" --> E["Non-array base type\n(e.g. name, int2vector)\n→ resolve via udt_name/format_type"]
    D -- true for real arrays --> F["Array type\n→ et.typname || '[]'"]
Loading

Reviews (1): Last reviewed commit: "fix: dump columns declared as `name` no ..." | Re-trigger Greptile

Comment thread ir/queries/queries.sql.go
@@ -830,7 +834,7 @@ type GetConstraintsRow struct {
Deferrable bool `db:"deferrable" json:"deferrable"`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 sql.NullBool may be unnecessary given COALESCE

Both GetConstraints and GetConstraintsForSchema use COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period in queries.sql, which guarantees a non-null result. sqlc emits sql.NullBool here likely because it can't statically prove nullability through the JSON operator chain — the access in inspector.go via .Bool is safe in practice (if Valid is somehow false, .Bool defaults to false, which is the correct "not temporal" value). No behaviour change, but it may be worth adding a -- @nullable:false hint (if your sqlc version supports it) or an sqlc.narg override to keep the generated type as plain bool and avoid the .Bool dereference.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect dumping of PostgreSQL name columns as char[] by tightening array-type detection in the column-type resolution query.

Changes:

  • Update column type resolution to detect arrays via pg_type.typcategory = 'A' instead of pg_type.typelem <> 0.
  • Regenerate/update sqlc artifacts and adjust constraint handling for is_period typing changes.
  • Add an integration test that reproduces Issue #410 (name type should not dump as char[]).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
ir/queries/queries.sql Refines array detection logic for resolved column types (fixes name misclassification).
ir/queries/queries.sql.go Updates generated query strings/types to match SQL changes; includes related regeneration diffs.
ir/inspector.go Adapts constraint-building to the updated generated type for IsPeriod.
cmd/dump/dump_integration_test.go Adds an integration test covering the name vs char[] regression.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ir/queries/queries.sql.go
InitiallyDeferred bool `db:"initially_deferred" json:"initially_deferred"`
IsValid bool `db:"is_valid" json:"is_valid"`
IsPeriod bool `db:"is_period" json:"is_period"`
IsPeriod sql.NullBool `db:"is_period" json:"is_period"`
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_period is selected with COALESCE(..., false) in the query, so it should be non-NULL. Changing IsPeriod from bool to sql.NullBool is likely an unintended API break (and forces call sites to use .Bool/.Valid). Consider rewriting the SQL expression to something that sqlc can infer as NOT NULL (e.g., ((to_jsonb(c) ->> 'conperiod')::boolean IS TRUE) AS is_period) and re-generating so this field stays a plain bool.

Suggested change
IsPeriod sql.NullBool `db:"is_period" json:"is_period"`
IsPeriod bool `db:"is_period" json:"is_period"`

Copilot uses AI. Check for mistakes.
Comment thread ir/queries/queries.sql.go
InitiallyDeferred bool `db:"initially_deferred" json:"initially_deferred"`
IsValid bool `db:"is_valid" json:"is_valid"`
IsPeriod bool `db:"is_period" json:"is_period"`
IsPeriod sql.NullBool `db:"is_period" json:"is_period"`
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above for the schema-scoped constraints row: is_period is computed via COALESCE(..., false) and should be non-NULL, so exposing it as sql.NullBool is likely unnecessary and potentially breaking for any consumers of this generated type. Consider adjusting the SQL to an expression sqlc infers as NOT NULL (e.g., ... IS TRUE) and re-generating so IsPeriod remains bool.

Suggested change
IsPeriod sql.NullBool `db:"is_period" json:"is_period"`
IsPeriod bool `db:"is_period" json:"is_period"`

Copilot uses AI. Check for mistakes.
@ivgiuliani
Copy link
Copy Markdown
Contributor Author

FWIW ir/queries/queries.sql.go is autogenerated by SQLc, I don't mind fixing these issues but they'll come back if/wehn the file is regenerated once again.

Copy link
Copy Markdown
Contributor

@tianzhou tianzhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tianzhou
Copy link
Copy Markdown
Contributor

Thanks for the contribution.

@tianzhou tianzhou merged commit 0a1f2b3 into pgplex:main Apr 27, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pgschema dump outputs char[] for columns declared as name

3 participants