feat(postgres): add opt-in unlogged table support#2468
feat(postgres): add opt-in unlogged table support#2468ReneWerner87 merged 5 commits intogofiber:mainfrom
Conversation
Allow creating the storage table as UNLOGGED for write-heavy cache workloads while preserving logged tables as the default behavior.
ofmendi
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @ofmendi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces an opt-in feature to create PostgreSQL tables as Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an opt-in feature for creating UNLOGGED tables in PostgreSQL, which is a valuable addition for cache-like workloads. The changes are well-structured, including updates to the configuration, documentation, and the addition of relevant tests to verify the new functionality. My review includes one critical security-related comment regarding a potential SQL injection vulnerability in how database queries are constructed, along with a suggested fix.
|
@ofmendi can you check the ai comments |
WalkthroughAdds a public Unlogged bool to Postgres storage Config, uses that option when creating tables (optionally CREATE UNLOGGED TABLE), switches init logic to sanitized, parameterized table/index creation, and adds tests verifying table persistence ("p" vs "u"). Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@postgres/postgres.go`:
- Around line 36-41: The hardcoded index name causes collisions; change
createIndexQuery so it accepts a dynamic index name (e.g. `CREATE INDEX IF NOT
EXISTS %s ON %s (e);`) and, where the index is created, construct a unique index
name from the table (e.g. take cfg.Table, replace dots/illegal chars with
underscores and prefix with "e_") then call fmt.Sprintf(createIndexQuery,
indexName, cfg.Table). Update references to createIndexQuery and the code path
that executes the index creation so each table gets its own index name.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
postgres/README.mdpostgres/config.gopostgres/postgres.gopostgres/postgres_test.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-01-30T12:29:41.774Z
Learnt from: the-real-i9
Repo: gofiber/storage PR: 1562
File: neo4j/neo4j.go:50-52
Timestamp: 2025-01-30T12:29:41.774Z
Learning: Storage drivers in the gofiber/storage repository intentionally panic on connection errors as a design choice to fail fast when storage is unavailable.
Applied to files:
postgres/config.gopostgres/README.md
📚 Learning: 2025-02-12T11:24:31.153Z
Learnt from: ReneWerner87
Repo: gofiber/storage PR: 0
File: :0-0
Timestamp: 2025-02-12T11:24:31.153Z
Learning: The storage package in gofiber/storage repository can be used independently of the Fiber web framework.
Applied to files:
postgres/config.gopostgres/README.md
📚 Learning: 2025-05-01T11:46:33.057Z
Learnt from: gaby
Repo: gofiber/storage PR: 1638
File: surrealdb/surrealdb.go:34-43
Timestamp: 2025-05-01T11:46:33.057Z
Learning: In the gofiber/storage package, storage drivers should panic when encountering database connection failures during initialization rather than returning errors. This is an intentional design pattern to ensure applications fail fast and explicitly when critical database dependencies are unavailable.
Applied to files:
postgres/config.gopostgres/README.md
📚 Learning: 2024-07-01T15:47:48.701Z
Learnt from: luk3skyw4lker
Repo: gofiber/storage PR: 1342
File: clickhouse/clickhouse.go:114-115
Timestamp: 2024-07-01T15:47:48.701Z
Learning: The pattern of returning `nil` for empty keys or values is consistent across different storage drivers in the gofiber/storage repository, including Redis, Rueidis, MongoDB, and Clickhouse.
Applied to files:
postgres/config.gopostgres/README.md
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: luk3skyw4lker
Repo: gofiber/storage PR: 1342
File: clickhouse/clickhouse.go:57-58
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The pattern of returning `nil` for empty keys or values is consistent across different storage drivers in the gofiber/storage repository, including Redis, Rueidis, and MongoDB.
Applied to files:
postgres/config.gopostgres/README.md
🧬 Code graph analysis (1)
postgres/postgres_test.go (1)
postgres/postgres.go (1)
Storage(16-26)
🔇 Additional comments (6)
postgres/config.go (1)
64-69: Well-documented opt-in field with a safe default.The
Unloggedfield correctly defaults tofalse(Go zero value), the warning about crash safety and replication is helpful, and no override inconfigDefault()is needed since the zero value is the desired default.postgres/postgres.go (2)
54-60: Clean helper with correct spacing.The trailing space in
"UNLOGGED "correctly producesCREATE UNLOGGED TABLEwhen combined with the format string on line 36.
109-118: Initialization logic for unlogged tables looks correct.The table-exists check guards creation,
createTableTypeClauseis applied only at creation time (matching the documented "applies only when the table is created" behavior), and the panic-on-error pattern is consistent with the repository's design conventions.postgres/postgres_test.go (2)
67-88: Well-structured test helper.Uses parameterized queries (avoiding SQL injection), correctly parses schema-qualified names with
SplitN, and directly queries the catalog for an authoritative answer.
229-248: Good test coverage for both logged and unlogged table creation.Both tests use unique table names for isolation and correctly assert the expected
relpersistencevalues ("p"for logged,"u"for unlogged).postgres/README.md (1)
121-127: Documentation accurately mirrors the code.The config block, default values, and warnings about crash safety and replication are consistent with the implementation in
config.go.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in configuration flag for creating PostgreSQL storage tables as UNLOGGED, enabling better performance for cache-like workloads while documenting the durability tradeoff.
Changes:
- Introduces
Config.Unlogged(defaultfalse) to create tables asUNLOGGEDon initialization. - Updates table/index creation logic to use a generated, sanitized full table name and a per-table index name.
- Adds tests asserting default (logged) vs. unlogged table persistence, and updates README/config docs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| postgres/postgres.go | Adds UNLOGGED clause support; refactors table/index creation and SQL strings to use sanitized identifiers. |
| postgres/config.go | Adds Unlogged config field and default value. |
| postgres/postgres_test.go | Adds helper + tests verifying relpersistence for logged vs. unlogged tables. |
| postgres/README.md | Documents Unlogged option, defaults, and warning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use query arguments instead of string interpolation for schema and table filters when checking table existence and validating table schema.
Generate a deterministic hashed index name to avoid identifier truncation collisions for long table names while keeping CREATE INDEX IF NOT EXISTS behavior stable.
Use explicit TABLE and UNLOGGED TABLE clauses to avoid whitespace-sensitive SQL assembly when building CREATE TABLE statements.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
postgres/postgres.go (1)
118-128: Consider wrapping both DDL statements in a single transaction to avoid a partially-initialised table.If
CREATE TABLEsucceeds butCREATE INDEXfails (e.g. out-of-disk, lock timeout), the table is persisted. On the next restarttableExistswill betrue, so the init block is skipped entirely and the table is left permanently un-indexed. Every GC run (DELETE … WHERE e <= $1 AND e != 0) will then do a full sequential scan for the lifetime of that table.PostgreSQL supports transactional DDL, so both statements can be rolled back atomically on failure:
♻️ Proposed fix: wrap DDL in an explicit transaction
- // Init database queries - if !tableExists { - for _, query := range []string{ - fmt.Sprintf(createTableQuery, createTableType(cfg.Unlogged), fullTableName), - fmt.Sprintf(createIndexQuery, indexName, fullTableName), - } { - if _, err := db.Exec(context.Background(), query); err != nil { - db.Close() - panic(err) - } - } - } + // Init database queries + if !tableExists { + tx, err := db.Begin(context.Background()) + if err != nil { + db.Close() + panic(err) + } + for _, query := range []string{ + fmt.Sprintf(createTableQuery, createTableType(cfg.Unlogged), fullTableName), + fmt.Sprintf(createIndexQuery, indexName, fullTableName), + } { + if _, err := tx.Exec(context.Background(), query); err != nil { + _ = tx.Rollback(context.Background()) + db.Close() + panic(err) + } + } + if err := tx.Commit(context.Background()); err != nil { + db.Close() + panic(err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@postgres/postgres.go` around lines 118 - 128, Wrap the two DDL statements (the fmt.Sprintf(createTableQuery, createTableType(cfg.Unlogged), fullTableName) and fmt.Sprintf(createIndexQuery, indexName, fullTableName)) in a single explicit transaction so they commit or roll back together: when tableExists is false, start a transaction on db (use the pool/conn's Begin/BeginTx to get tx), execute both queries via tx.Exec (not db.Exec), and if any Exec returns an error call tx.Rollback and handle/propagate the error; only call tx.Commit on success and avoid leaving the table created without its index.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
postgres/postgres.go
🔇 Additional comments (3)
postgres/postgres.go (3)
56-62: LGTM –createTableTypecleanly encodes the DDL variant.Returning the full
"UNLOGGED TABLE"/"TABLE"fragment avoids any whitespace-sensitive string assembly at the call site, exactly as described in the commit message. Both produce valid PostgreSQL DDL when combined with the sanitisedfullTableName.
34-48: Parameterizedinformation_schemalookups are a solid security improvement.Replacing
fmt.Sprintfstring interpolation incheckTableExistsQueryandcheckSchemaQuerywith$1/$2bind parameters eliminates any residual injection risk and is idiomatic pgx v5. LGTM.
96-97:pgx.Identifier.Sanitize()usage is correct for both identifiers.
fullTableName(2-element slice) →"schema"."table": proper schema-qualified DDL/DML.indexName(1-element slice) →"idx_e_…": no schema prefix is needed — PostgreSQL automatically creates the index in the table's own schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@postgres/postgres.go`:
- Around line 64-67: The buildIndexName change correctly replaces the hardcoded
"e" with a SHA-256-derived suffix; leave the implementation of buildIndexName
as-is (it uses sha256.Sum256 and hex.EncodeToString(hash[:6]) to produce
"idx_e_"+12-hex-char suffix), no further changes required since the resulting
identifier length is well under PostgreSQL's 63-char limit and collision risk is
negligible.
---
Nitpick comments:
In `@postgres/postgres.go`:
- Around line 118-128: Wrap the two DDL statements (the
fmt.Sprintf(createTableQuery, createTableType(cfg.Unlogged), fullTableName) and
fmt.Sprintf(createIndexQuery, indexName, fullTableName)) in a single explicit
transaction so they commit or roll back together: when tableExists is false,
start a transaction on db (use the pool/conn's Begin/BeginTx to get tx), execute
both queries via tx.Exec (not db.Exec), and if any Exec returns an error call
tx.Rollback and handle/propagate the error; only call tx.Commit on success and
avoid leaving the table created without its index.
Hello everyone,
PostgreSQL offers UNLOGGED tables, which can be great for cache style workloads. While looking through the source, I noticed this isn’t currently configurable, so I thought it would be a nice improvement to support it.
Just a small note: UNLOGGED tables are not crash-safe, so if durability is required.
Summary by CodeRabbit
New Features
Documentation
Tests