Add postgres Store - #975
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c5278ae6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR introduces the initial PostgreSQL-backed persistence layer for hostd, including schema initialization, migration scaffolding, and embedded-Postgres-based tests to validate schema/migration consistency.
Changes:
- Added a new
persist/postgrespackage with store open/init logic, transaction retry/backoff handling, and SQL helpers for slow-query logging. - Introduced an initial Postgres schema (
init.sql) plus migration scaffolding (migrations.go) and a migration consistency test using an embedded Postgres instance. - Added new dependencies for pgx and embedded Postgres in
go.mod/go.sum.
Note: this repo contains knope.toml; consider adding a knope changeset for this PR to document the addition of the Postgres store.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| persist/postgres/store.go | Store open/close, database creation, and transaction retry/backoff wrapper. |
| persist/postgres/sql.go | *sql.Tx/*sql.Stmt wrappers for slow-query logging and DB version helpers. |
| persist/postgres/migrations.go | Migration list placeholder (starts at v1, currently no migrations). |
| persist/postgres/migrations_test.go | Embedded Postgres test harness and schema consistency checks. |
| persist/postgres/init.sql | Initial Postgres schema for hostd persistence tables. |
| persist/postgres/init.go | Init/upgrade flow, schema application, db_version management, host key generation. |
| go.mod | Adds pgx + embedded-postgres dependencies. |
| go.sum | Dependency checksums for the new modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1a3816d to
25a0631
Compare
12b8901 to
e86a388
Compare
4867672 to
07e01ab
Compare
07e01ab to
4705cfa
Compare
| sector_writes BIGINT NOT NULL DEFAULT 0, | ||
| UNIQUE (volume_id, volume_index) | ||
| ); | ||
| CREATE INDEX volume_sectors_sector_writes_volume_id_sector_id_volume_index_compound ON volume_sectors(sector_writes ASC, volume_id, sector_id, volume_index) WHERE sector_id IS NULL; |
There was a problem hiding this comment.
do we care this index name exceeds the 63 char limit postgres imposes? (there's 3 in total)
|
|
||
| // getDBVersion returns the current version of the database. | ||
| func getDBVersion(ctx context.Context, pool *pgxpool.Pool) (version int64) { | ||
| // error is ignored -- the database may not have been initialized yet. |
There was a problem hiding this comment.
this is kind of odd? we do this for s3d and indexd too but wouldn't it be better to check explicitly and return an error if it's not initialized? or perhaps even retry with a small backoff and error out after a second or so
| cached_subtree_roots BYTEA, | ||
| last_access_timestamp TIMESTAMP WITH TIME ZONE NOT NULL | ||
| ); | ||
| CREATE INDEX stored_sectors_sector_root ON stored_sectors(sector_root); |
There was a problem hiding this comment.
We don't need an index here because we already have UNIQUE on that column. It seems there are a few more places this happens
This PR is the first of probably quite a few to introduce a Postgres driver to hostd.
It starts by adding a
init.sqlwhich is similar to SQLite's but with Postgres types as well as similar boilerplate code we use inindexd. The only difference betweenindexdandhostdis that this PR uses an embedded Postgres db for testing which makes running tests more convenient.Since we have 100+ store interface methods the goal is to follow up with at least 1 PR per store interface in the host but considering some of those are quite large probably a lot more than that.
I really want to avoid this being a dumb translation and instead make sure it's making good use of Postgres' capabilities.