Conversation
Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds PostgreSQL database support alongside the existing SQLite implementation, allowing runtime selection via environment variables. However, the implementation has several critical issues that prevent the PostgreSQL driver from functioning correctly.
Changes:
- Added new PostgreSQL driver in
lib/database/drivers/postgres/with environment-based configuration - Modified database wrapper to support runtime driver selection via
DB_DRIVERenvironment variable - Updated database migrations to use cross-compatible data types (BYTEA, TIMESTAMP, single-quoted strings)
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/database/wrapper.go | Added driver selection logic based on DB_DRIVER environment variable, supporting "sqlite", "postgres", and "postgresql" values |
| lib/database/drivers/postgres/postgres.go | New PostgreSQL driver implementation with environment-based connection configuration |
| lib/database/query.sql.go | SQLC version bump from v1.29.0 to v1.30.0 |
| lib/database/models.go | SQLC version bump from v1.29.0 to v1.30.0 |
| lib/database/db.go | SQLC version bump from v1.29.0 to v1.30.0 |
| database/migrations/0001_init.up.sql | Changed BLOB→BYTEA, DATETIME→TIMESTAMP, fixed string literals from double to single quotes |
| database/migrations/0002_add_fields.up.sql | Changed BLOB→BYTEA, DATETIME→TIMESTAMP, fixed foreign key reference from sample_tags to tags |
| go.mod | Added github.com/lib/pq v1.10.9 dependency for PostgreSQL driver |
| go.sum | Added dependencies for PostgreSQL driver and related testing libraries |
| README.md | Added PostgreSQL configuration documentation with environment variable examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Implement some form of local-file postgres (embedded) that stores to a local file in place of trying to support both postgresSQL and sqlite. Only use the embedded if an external postgres is not specified. Also setup a migrations table to store the down queries and execute them if a rollback is performed (compare latest migration vs the migrations table). |
I need clarification on the embedded PostgreSQL requirement. True embedded PostgreSQL (like embedded-postgres-go) still runs a full PostgreSQL server process in the background - it doesn't work like SQLite with a simple file-based storage. Would you prefer:
For the migration rollback functionality, I can implement a custom migrations table to store down queries regardless of which approach we choose. |
|
@copilot is there not a solution where the data for the embedded process can be stored in the database directory that currently stores the db.sqlite? |
Yes, there are solutions that can store data in the database directory:
Given the bot review identified that current PostgreSQL support doesn't work (queries use SQLite syntax), I recommend:
Would you like me to proceed with this approach, or try embedded-postgres-go with data in the database directory? |
…SQL syntax Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
…l PostgreSQL Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
Co-authored-by: reesvarney <22414669+reesvarney@users.noreply.github.com>
…ithout error handling' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…esvarney/sample-tracker into copilot/add-postgresql-driver
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…or Description and PartNumber
|
@reesvarney Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
2 similar comments
|
@reesvarney Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
|
@reesvarney Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
This PR migrates the database layer to Postgres, enabling better control over data storage.
Required changes:
main.goto prevent locked database connections