Skip to content

Enhance SQL query structure and formatting #425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Rooffeell wants to merge 1 commit into from
+27 −46
Closed

Enhance SQL query structure and formatting #425

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 27 additions & 46 deletions ir/queries/queries.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ WHERE

-- GetTables retrieves all tables in the database with metadata
-- name: GetTables :many
SELECT
SELECT
t.table_schema,
t.table_name,
t.table_type,
COALESCE(d.description, '') AS table_comment,
c.relpersistence::text AS relpersistence
COALESCE(d.description, '') AS table_comment
Comment on lines +25 to +29
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 30, 2026

Choose a reason for hiding this comment

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

P0 Generated Go code not regenerated after SQL changes

queries.sql.go was not re-generated with sqlc generate after this PR's changes. This has two consequences: (1) the SQL changes here are entirely inert at runtime — the app still executes the old queries from the embedded string constants in the generated file, and (2) running sqlc generate after merging will produce a Go file that fails to compile because inspector.go still references fields that are being removed:

  • inspector.go:264 — table.Relpersistence (removed from GetTables/GetTablesForSchema)
  • inspector.go:495 — constraint.NoInherit (removed from both constraints queries)
  • inspector.go:497 — constraint.NullsNotDistinct.Bool (removed from GetConstraintsForSchema)
  • inspector.go:773 — indexRow.IncludeColumns (removed from GetIndexesForSchema)

queries.sql.go must be regenerated and inspector.go must be updated to stop referencing the removed fields before this PR is mergeable.

Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 30, 2026

Choose a reason for hiding this comment

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

P1 relpersistence removal breaks UNLOGGED table detection

Removing c.relpersistence::text AS relpersistence from both GetTables and GetTablesForSchema will drop the ability to detect UNLOGGED tables. inspector.go:264 reads:

Unlogged: table.Relpersistence.Valid && table.Relpersistence.String == "u",

Without this column, Relpersistence would always be the zero value and every table would appear as a regular (logged) table, causing UNLOGGED tables to lose their UNLOGGED modifier in dumps and diffs.

FROM information_schema.tables t
LEFT JOIN pg_namespace n ON n.nspname = t.table_schema
LEFT JOIN pg_class c ON c.relname = t.table_name AND c.relnamespace = n.oid
Expand All @@ -45,8 +44,7 @@ SELECT
t.table_schema,
t.table_name,
t.table_type,
COALESCE(d.description, '') AS table_comment,
c.relpersistence::text AS relpersistence
COALESCE(d.description, '') AS table_comment
FROM information_schema.tables t
LEFT JOIN pg_namespace n ON n.nspname = t.table_schema
LEFT JOIN pg_class c ON c.relname = t.table_name AND c.relnamespace = n.oid
Expand Down Expand Up @@ -74,21 +72,19 @@ WITH column_base AS (
COALESCE(d.description, '') AS column_comment,
CASE
WHEN dt.typtype = 'd' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
CASE WHEN dn.nspname = c.table_schema THEN format_type(dt.oid, NULL)
ELSE dn.nspname || '.' || format_type(dt.oid, NULL)
END
WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
CASE WHEN dn.nspname = c.table_schema THEN format_type(dt.oid, NULL)
ELSE dn.nspname || '.' || format_type(dt.oid, NULL)
END
WHEN dt.typtype = 'b' AND dt.typcategory = 'A' THEN
WHEN dt.typtype = 'b' AND dt.typelem <> 0 THEN
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 30, 2026

Choose a reason for hiding this comment

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

P1 Array detection condition is a documented regression

The condition was changed from dt.typcategory = 'A' to dt.typelem <> 0, but the original code had an explicit comment explaining exactly why typelem <> 0 is wrong:

Use typcategory = 'A' rather than typelem <> 0; the latter is true for non-array fixed-length types like name (typelem points to char).

The name type (pg_catalog.name) has typelem = 18 (pointing to char), but typcategory = 'S' (string), not 'A'. With this change, columns of type name would hit the array branch and be misidentified as char[]. The same incorrect condition appears at line 198 in GetColumnsForSchema.

Suggested change
WHEN dt.typtype = 'b' AND dt.typelem <> 0 THEN
WHEN dt.typtype = 'b' AND dt.typcategory = 'A' THEN

-- Array types: apply same schema qualification logic to element type
-- Use typcategory = 'A' rather than typelem <> 0; the latter is true
-- for non-array fixed-length types like name (typelem points to char).
CASE
WHEN en.nspname = 'pg_catalog' THEN et.typname || '[]'
WHEN en.nspname = c.table_schema THEN et.typname || '[]'
ELSE en.nspname || '.' || et.typname || '[]'
WHEN en.nspname = 'pg_catalog' THEN format_type(et.oid, NULL) || '[]'
WHEN en.nspname = c.table_schema THEN format_type(et.oid, NULL) || '[]'
ELSE en.nspname || '.' || format_type(et.oid, NULL) || '[]'
END
WHEN dt.typtype = 'b' THEN
-- Non-array base types: qualify if not in pg_catalog or table's schema
Expand Down Expand Up @@ -192,21 +188,19 @@ WITH column_base AS (
COALESCE(d.description, '') AS column_comment,
CASE
WHEN dt.typtype = 'd' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
CASE WHEN dn.nspname = c.table_schema THEN format_type(dt.oid, NULL)
ELSE dn.nspname || '.' || format_type(dt.oid, NULL)
END
WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
CASE WHEN dn.nspname = c.table_schema THEN format_type(dt.oid, NULL)
ELSE dn.nspname || '.' || format_type(dt.oid, NULL)
END
WHEN dt.typtype = 'b' AND dt.typcategory = 'A' THEN
WHEN dt.typtype = 'b' AND dt.typelem <> 0 THEN
-- Array types: apply same schema qualification logic to element type
-- Use typcategory = 'A' rather than typelem <> 0; the latter is true
-- for non-array fixed-length types like name (typelem points to char).
CASE
WHEN en.nspname = 'pg_catalog' THEN et.typname || '[]'
WHEN en.nspname = c.table_schema THEN et.typname || '[]'
ELSE en.nspname || '.' || et.typname || '[]'
WHEN en.nspname = 'pg_catalog' THEN format_type(et.oid, NULL) || '[]'
WHEN en.nspname = c.table_schema THEN format_type(et.oid, NULL) || '[]'
ELSE en.nspname || '.' || format_type(et.oid, NULL) || '[]'
END
WHEN dt.typtype = 'b' THEN
-- Non-array base types: qualify if not in pg_catalog or table's schema
Expand Down Expand Up @@ -344,8 +338,7 @@ SELECT
c.condeferrable AS deferrable,
c.condeferred AS initially_deferred,
c.convalidated AS is_valid,
COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period,
c.connoinherit AS no_inherit
COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 30, 2026

Choose a reason for hiding this comment

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

P1 Removal of no_inherit and nulls_not_distinct drops active features

Both GetConstraints (line 341) and GetConstraintsForSchema (line 920) have had c.connoinherit AS no_inherit removed. Additionally GetConstraintsForSchema lost nulls_not_distinct and its LEFT JOIN pg_index i. These fields drive active behaviour:

  • inspector.go:495 populates Constraint.NoInherit — removing this silently makes all CHECK constraints appear inheritable, producing incorrect DDL for any schema that uses NO INHERIT checks.
  • inspector.go:497 populates Constraint.NullsNotDistinct (PG15+ UNIQUE NULLS NOT DISTINCT) — removing this will generate incorrect migration diffs for unique constraints that use NULLS NOT DISTINCT.

Both fields need to be retained, or their consumers in inspector.go must be updated accordingly.

FROM pg_constraint c
JOIN pg_class cl ON c.conrelid = cl.oid
JOIN pg_namespace n ON cl.relnamespace = n.oid
Expand Down Expand Up @@ -443,32 +436,27 @@ WITH index_base AS (
ELSE false
END as has_expressions,
COALESCE(d.description, '') AS index_comment,
idx.indnkeyatts as num_key_columns,
idx.indnatts as num_columns,
ARRAY(
SELECT pg_get_indexdef(idx.indexrelid, k::int, true)
FROM generate_series(1, idx.indnkeyatts) k
FROM generate_series(1, idx.indnatts) k
) as column_definitions,
ARRAY(
SELECT
CASE
WHEN (idx.indoption[k-1] & 1) = 1 THEN 'DESC'
ELSE 'ASC'
END
FROM generate_series(1, idx.indnkeyatts) k
FROM generate_series(1, idx.indnatts) k
) as column_directions,
ARRAY(
SELECT CASE
WHEN opc.opcdefault THEN '' -- Omit default operator classes
ELSE COALESCE(opc.opcname, '')
END
FROM generate_series(1, idx.indnkeyatts) k
FROM generate_series(1, idx.indnatts) k
LEFT JOIN pg_opclass opc ON opc.oid = idx.indclass[k-1]
Comment on lines +450 to 458
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 30, 2026

Choose a reason for hiding this comment

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

P1 INCLUDE columns incorrectly included in direction and opclass arrays

column_directions and column_opclasses now iterate over indnatts (all columns including INCLUDE columns) instead of indnkeyatts (key columns only). INCLUDE columns have no sort order and no operator class — they are not indexing key columns, just stored values. For an index like CREATE INDEX ON t (a) INCLUDE (b), this will generate ['ASC', 'ASC'] for directions and look up an indclass entry that is 0/undefined for the INCLUDE column slot.

The indoption and indclass arrays are only defined for the first indnkeyatts positions; entries beyond that point are meaningless. The include_columns array (now removed) was the correct place to handle the INCLUDE columns separately.

) as column_opclasses,
ARRAY(
SELECT pg_get_indexdef(idx.indexrelid, k::int, true)
FROM generate_series(idx.indnkeyatts + 1, idx.indnatts) k
) as include_columns
) as column_opclasses
FROM pg_index idx
JOIN pg_class i ON i.oid = idx.indexrelid
JOIN pg_class t ON t.oid = idx.indrelid
Expand Down Expand Up @@ -500,12 +488,10 @@ SELECT
sp.partial_predicate,
ib.has_expressions,
ib.index_comment,
ib.num_key_columns,
ib.num_columns,
ib.column_definitions,
ib.column_directions,
ib.column_opclasses,
ib.include_columns
ib.column_opclasses
FROM index_base ib
CROSS JOIN LATERAL (
SELECT
Expand Down Expand Up @@ -931,19 +917,14 @@ SELECT
c.condeferrable AS deferrable,
c.condeferred AS initially_deferred,
c.convalidated AS is_valid,
COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period,
c.connoinherit AS no_inherit,
-- pg_index.indnullsnotdistinct is PG15+. Use to_jsonb so the column reference
-- doesn't fail to plan on PG14 (where the attribute does not exist on pg_index).
COALESCE((to_jsonb(i) ->> 'indnullsnotdistinct')::boolean, false) AS nulls_not_distinct
COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period
FROM pg_constraint c
JOIN pg_class cl ON c.conrelid = cl.oid
JOIN pg_namespace n ON cl.relnamespace = n.oid
LEFT JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = ANY(c.conkey)
LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid
LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid
LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)]
LEFT JOIN pg_index i ON i.indexrelid = c.conindid
WHERE n.nspname = $1
ORDER BY n.nspname, cl.relname, c.contype, c.conname, a.attnum;

Expand Down Expand Up @@ -1446,4 +1427,4 @@ JOIN pg_namespace referenced_ns ON referenced_proc.pronamespace = referenced_ns.
WHERE d.classid = 'pg_proc'::regclass
AND d.refclassid = 'pg_proc'::regclass
AND d.deptype = 'n'
AND dependent_ns.nspname = $1;
AND dependent_ns.nspname = $1;