Skip to content

Add support for UNLOGGED tables#424

Merged
tianzhou merged 3 commits intopgplex:mainfrom
ivgiuliani:unlogged
Apr 30, 2026
Merged

Add support for UNLOGGED tables#424
tianzhou merged 3 commits intopgplex:mainfrom
ivgiuliani:unlogged

Conversation

@ivgiuliani
Copy link
Copy Markdown
Contributor

This is currently dropped by table definitions, although UNLOGGED makes the table behave differently wrt replication and persistence (docs)

Fix #423

ivgiuliani and others added 2 commits April 29, 2026 16:00
Adds three fixtures under testdata/diff/create_table/ covering
`UNLOGGED` tables:

- add_table_unlogged: empty schema -> CREATE UNLOGGED TABLE
- set_unlogged: LOGGED table -> ALTER TABLE ... SET UNLOGGED
- set_logged: UNLOGGED table -> ALTER TABLE ... SET LOGGED

All currently fail because pgschema does not track `UNLOGGED` in the DDL.
Adds c.relpersistence::text to GetTables and GetTablesForSchema and
regenerates the sqlc output (sqlc 1.30) so the inspector can read
table persistence ('p' permanent, 'u' unlogged) when building the IR.
No behavior change yet; the new field is wired up in a follow-up
commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR adds support for UNLOGGED tables by reading pg_class.relpersistence during inspection, propagating the Unlogged flag through the IR, and emitting CREATE UNLOGGED TABLE for new tables and ALTER TABLE … SET LOGGED/UNLOGGED for persistence changes. Three new test cases cover the create, set-logged, and set-unlogged scenarios.

Confidence Score: 4/5

Safe to merge; only P2 style findings, no correctness or runtime issues.

The implementation is correct end-to-end: inspection, diffing, SQL generation, and plan rendering all handle UNLOGGED properly. Only two minor style issues found (comment typo and an implicit fallback in extractTablePathFromSubResource).

No files require special attention.

Important Files Changed

Filename Overview
internal/diff/diff.go Adds DiffTypeTablePersistence constant, its String() and UnmarshalJSON cases, and PersistenceChanged field to tableDiff; all symmetric with existing diff types.
internal/diff/table.go Adds persistence diff detection and ALTER TABLE … SET LOGGED/UNLOGGED emission. Logic reads newTable.Unlogged (via td.Table) to pick the clause, which is correct. Minor comment typo ("viceversa").
ir/inspector.go Sets Unlogged from relpersistence == "u"; correctly guards the NullString.Valid check so the field defaults to false when the join produces no row.
ir/queries/queries.sql Adds c.relpersistence::text to both GetTables and GetTablesForSchema queries via the existing pg_class LEFT JOIN; also adds a trailing newline to sqlc.yaml.
ir/queries/queries.sql.go Generated file updated consistently with the SQL changes; Relpersistence sql.NullString added to both row structs and scanned in both query functions.
internal/plan/plan.go table.persistence works via the generic 2-part fallback in extractTablePathFromSubResource, but is not listed alongside table.rls / table.comment in the explicit check — functional but inconsistent.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pg_class.relpersistence] -->|inspected via LEFT JOIN| B[ir.Table.Unlogged bool]
    B --> C{Diff: old vs new}
    C -->|New table added| D[generateTableSQL]
    D -->|Unlogged=true| E[CREATE UNLOGGED TABLE IF NOT EXISTS ...]
    D -->|Unlogged=false| F[CREATE TABLE IF NOT EXISTS ...]
    C -->|Existing table changed| G[tableDiff.PersistenceChanged=true]
    G --> H[generateAlterTableStatements]
    H -->|New.Unlogged=true| I[ALTER TABLE x SET UNLOGGED]
    H -->|New.Unlogged=false| J[ALTER TABLE x SET LOGGED]
    I --> K[DiffTypeTablePersistence / CanRunInTransaction=true]
    J --> K
Loading

Comments Outside Diff (1)

  1. internal/plan/plan.go, line 1128 (link)

    P2 table.persistence not explicitly listed in extractTablePathFromSubResource

    "table.rls" and "table.comment" are explicitly listed as "table-level changes" whose path is already the table path. The new "table.persistence" type has the same property (path is "schema.table", 2 parts) and falls through the generic 2-part fallback — which currently returns the correct result — but the intent isn't as clear. Consider adding it alongside the other table-level types for consistency and to guard against future path-format changes.

    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!

Reviews (1): Last reviewed commit: "fix: preserve UNLOGGED table persistence..." | Re-trigger Greptile

Comment thread internal/diff/table.go
pgschema previously emitted plain `CREATE TABLE` for both `LOGGED` and
`UNLOGGED` tables, and silently no-op'd persistence transitions.

This wires `UNLOGGED` end-to-end: the inspector now reads `relpersistence`
into a new `ir.Table.Unlogged field`, the table-diff detects persistence
flips, and the SQL emitter produces the relevant `UNLOGGED` modifiers
(incl `ALTER TABLE ... SET LOGGED/UNLOGGED` for transitions).

The persistence change is emitted before column and constraint alterations
because PostgreSQL rewrites the heap on the toggle, so doing it first
minimizes downstream data movement.
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

Adds first-class support for PostgreSQL UNLOGGED tables so that schema inspection/dumps and generated migrations preserve and apply table persistence correctly (fixes #423).

Changes:

  • Extend table inspection to capture persistence via pg_class.relpersistence and store it on ir.Table as Unlogged.
  • Teach table DDL generation to emit CREATE UNLOGGED TABLE ... and to diff persistence changes via ALTER TABLE ... SET LOGGED/UNLOGGED (new diff type table.persistence).
  • Add golden testdata cases covering create/unlogged and logged↔unlogged transitions.

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ir/ir.go Adds Table.Unlogged to IR to represent UNLOGGED tables.
ir/queries/queries.sql Selects pg_class.relpersistence for tables so persistence can be inspected.
ir/queries/queries.sql.go Regenerates sqlc output structs/methods to include relpersistence.
ir/queries/sqlc.yaml Minor config formatting adjustment (no functional change).
ir/inspector.go Sets Table.Unlogged based on relpersistence == 'u'.
internal/diff/diff.go Introduces DiffTypeTablePersistence (table.persistence) for plan/JSON typing.
internal/diff/table.go Emits CREATE UNLOGGED TABLE and generates ALTER TABLE ... SET LOGGED/UNLOGGED when persistence changes.
testdata/diff/create_table/add_table_unlogged/* Golden outputs for creating an unlogged table.
testdata/diff/create_table/set_unlogged/* Golden outputs for changing a table to UNLOGGED.
testdata/diff/create_table/set_logged/* Golden outputs for changing a table back to LOGGED.

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

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. Thanks for the contribution

@tianzhou tianzhou merged commit e4f3a12 into pgplex:main Apr 30, 2026
5 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.

UNLOGGED is dropped from table definitions

3 participants