Skip to content

feat: Add PostgreSQL 18 driver + driver refactor#33

Open
Unbreathable wants to merge 1 commit intomainfrom
feat/postgres-18
Open

feat: Add PostgreSQL 18 driver + driver refactor#33
Unbreathable wants to merge 1 commit intomainfrom
feat/postgres-18

Conversation

@Unbreathable
Copy link
Contributor

Description

  • This MR adds a PostgreSQL driver in the new pkg/databases directory for drivers that supports PostgreSQL 18 and its new container layout
  • The basic container lifecycle has been moved to a new mservices package for more easily creating drivers
  • The PostgreSQL legacy driver has been upgraded to use the new API (will be maintained until PostgreSQL 20 is out)
  • The real-project example has been updated to use the new PostgreSQL 18 driver

@Unbreathable Unbreathable added this to the Magic v3 milestone Mar 5, 2026
@Unbreathable Unbreathable self-assigned this Mar 5, 2026
@Unbreathable Unbreathable linked an issue Mar 5, 2026 that may be closed by this pull request
@Unbreathable Unbreathable requested a review from Copilot March 5, 2026 13:37
Copy link
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 a new PostgreSQL 18 service driver under pkg/databases/postgres, refactors Docker container lifecycle logic into reusable mrunner/services helpers, and migrates the legacy driver + real-project example to the new API.

Changes:

  • Introduce new PostgreSQL 18 driver (pkg/databases/postgres) with container lifecycle, health checks, initialization, and instruction handling.
  • Add mrunner/services helpers for managed container create/recreate + container exec, and update the legacy driver to use them.
  • Update examples/real-project to use the new PostgreSQL 18 driver.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
pkg/databases/postgres/postgres.go New PostgreSQL 18 driver implementation and config helpers
pkg/databases/postgres/postgres_container.go Container creation/health/init for new driver via mservices
pkg/databases/postgres/postgres_instruct.go Clear/drop table instructions for new driver
pkg/databases/postgres/go.mod New submodule definition for postgres driver
pkg/databases/postgres/go.sum Dependency locks for new postgres submodule
pkg/databases/postgres-legacy/postgres_legacy_container.go Refactored legacy container lifecycle to use mservices helpers
pkg/databases/postgres-legacy/postgres_legacy.go Updated legacy driver docs/comments
pkg/databases/postgres-legacy/go.mod Dependency cleanup / reclassification (direct -> indirect)
mrunner/services/containers.go New shared “managed container” creation/recreate logic
mrunner/services/containers_exec.go New shared helper to exec commands in containers
go.work Add postgres driver submodule to workspace
examples/real-project/starter/config.go Switch example to new postgres (18) driver
examples/real-project/go.mod Switch example module dependency/replace to new postgres driver

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

Env: []string{
fmt.Sprintf("POSTGRES_PASSWORD=%s", PostgresPassword),
fmt.Sprintf("POSTGRES_USER=%s", PostgresUsername),
"POSTGRES_DATABASE=postgres",
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The official postgres image uses POSTGRES_DB (not POSTGRES_DATABASE) to set the initial database name. With standard images this env var will be ignored. Consider switching to POSTGRES_DB=postgres (or removing it if the default is sufficient).

Suggested change
"POSTGRES_DATABASE=postgres",
"POSTGRES_DB=postgres",

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +5
module github.com/Liphium/magic/pkg/databases/postgres

go 1.25.7

replace github.com/Liphium/magic/v3 => ../../../.
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The module path in this new submodule doesn’t match how it’s imported elsewhere (e.g. github.com/Liphium/magic/v3/pkg/databases/postgres). With the current module github.com/Liphium/magic/pkg/databases/postgres, go will error with “module declares its path as … but was required as …” when consumers require/import the /v3/... path. Update the module line to the exact import path you expect users to require (likely github.com/Liphium/magic/v3/pkg/databases/postgres) and keep go.work/consumer replace directives aligned to it.

Copilot uses AI. Check for mistakes.
Comment on lines 5 to 12
replace github.com/Liphium/magic/v3 => ../../.

replace github.com/Liphium/magic/v3/pkg/databases/postgres-legacy => ../../pkg/databases/postgres-legacy
replace github.com/Liphium/magic/v3/pkg/databases/postgres => ../../pkg/databases/postgres

require (
github.com/Liphium/magic/v3 v3.0.0-00010101000000-000000000000
github.com/Liphium/magic/v3/pkg/databases/postgres-legacy v0.0.0-00010101000000-000000000000
github.com/Liphium/magic/v3/pkg/databases/postgres v0.0.0-00010101000000-000000000000
github.com/gofiber/fiber/v2 v2.52.9
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

This example requires/replaces github.com/Liphium/magic/v3/pkg/databases/postgres, but the pkg/databases/postgres submodule’s go.mod currently declares a different module path. As-is, go mod resolution will fail with a module-path mismatch error. Align the driver submodule module path (and/or this require/replace) so they match exactly.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +22
return client.ExecInspectResult{}, fmt.Errorf("couldn't create command for readiness of container: %s", err)
}
execStartCheck := client.ExecStartOptions{Detach: false, TTY: false}
if _, err := c.ExecStart(ctx, execIDResp.ID, execStartCheck); err != nil {
return client.ExecInspectResult{}, fmt.Errorf("couldn't start command for readiness of container: %s", err)
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

ExecuteCommand is a generic helper, but all error messages mention “readiness of container”. This makes debugging confusing when the helper is used for non-readiness execs. Consider changing these error strings to be command-agnostic (e.g. “couldn't create exec”, “couldn't start exec”).

Suggested change
return client.ExecInspectResult{}, fmt.Errorf("couldn't create command for readiness of container: %s", err)
}
execStartCheck := client.ExecStartOptions{Detach: false, TTY: false}
if _, err := c.ExecStart(ctx, execIDResp.ID, execStartCheck); err != nil {
return client.ExecInspectResult{}, fmt.Errorf("couldn't start command for readiness of container: %s", err)
return client.ExecInspectResult{}, fmt.Errorf("couldn't create exec: %s", err)
}
execStartCheck := client.ExecStartOptions{Detach: false, TTY: false}
if _, err := c.ExecStart(ctx, execIDResp.ID, execStartCheck); err != nil {
return client.ExecInspectResult{}, fmt.Errorf("couldn't start exec: %s", err)

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +58
// Get all of the tables
res, err := conn.Query("SELECT table_name FROM information_schema.tables WHERE table_schema NOT IN ('pg_catalog', 'information_schema')")
if err != nil {
return fmt.Errorf("couldn't get database tables: %v", err)
}
for res.Next() {
var name string
if err := res.Scan(&name); err != nil {
return fmt.Errorf("couldn't get database table name: %v", err)
}
if err := fn(name, conn); err != nil {
return err
}
}

return nil
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

iterateTables doesn’t close the Rows result or check Rows.Err() after iteration. This can leak resources and can silently ignore iteration errors. Add a defer res.Close() after a successful Query, and check res.Err() after the for res.Next() loop.

Copilot uses AI. Check for mistakes.
Env: []string{
fmt.Sprintf("POSTGRES_PASSWORD=%s", PostgresPassword),
fmt.Sprintf("POSTGRES_USER=%s", PostgresUsername),
"POSTGRES_DATABASE=postgres",
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The official postgres image uses POSTGRES_DB (not POSTGRES_DATABASE) to set the initial database name. As written, this env var will be ignored by standard images, which is confusing and makes it look like the DB name is configurable when it isn’t. Consider switching to POSTGRES_DB=postgres (or dropping it entirely since postgres is the default).

Suggested change
"POSTGRES_DATABASE=postgres",
"POSTGRES_DB=postgres",

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +72
return pd.iterateTables(container, func(tableName string, conn *sql.DB) error {
if _, err := conn.Exec(fmt.Sprintf("truncate %s CASCADE", tableName)); err != nil {
return fmt.Errorf("couldn't truncate table %s: %v", tableName, err)
}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

tableName is interpolated directly into the TRUNCATE statement. Even though it comes from information_schema, unquoted identifiers can break on mixed-case/reserved-word table names and can be abused if a table name contains SQL metacharacters. Use proper identifier quoting (e.g. quote_ident) and/or include schema + table and quote both parts before executing.

Copilot uses AI. Check for mistakes.
Comment on lines +79 to +82
return pd.iterateTables(container, func(tableName string, conn *sql.DB) error {
if _, err := conn.Exec(fmt.Sprintf("DROP TABLE %s CASCADE", tableName)); err != nil {
return fmt.Errorf("couldn't drop table table %s: %v", tableName, err)
}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

tableName is interpolated directly into the DROP TABLE statement. Unquoted identifiers can fail for many valid table names (reserved words, mixed case, special chars) and can allow SQL injection if a maliciously named table exists. Quote identifiers (and ideally schema + table) before executing.

Copilot uses AI. Check for mistakes.
func (pd *PostgresDriver) DropTables(container mconfig.ContainerInformation) error {
return pd.iterateTables(container, func(tableName string, conn *sql.DB) error {
if _, err := conn.Exec(fmt.Sprintf("DROP TABLE %s CASCADE", tableName)); err != nil {
return fmt.Errorf("couldn't drop table table %s: %v", tableName, err)
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

Typo in error message: duplicated word “table”.

Suggested change
return fmt.Errorf("couldn't drop table table %s: %v", tableName, err)
return fmt.Errorf("couldn't drop table %s: %v", tableName, err)

Copilot uses AI. Check for mistakes.
"strings"

"github.com/Liphium/magic/v3/mconfig"
"github.com/Liphium/magic/v3/util"
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

This driver calls sql.Open("postgres", ...) but the package doesn’t import/register any Postgres database/sql driver (e.g. blank-importing github.com/lib/pq). Right now it implicitly relies on some other package (like mrunner) to have side-effect imports, which makes the driver easy to use incorrectly and can lead to runtime sql: unknown driver "postgres" errors in other integration paths. Consider adding the blank import here (and adding the dependency in this submodule) so the driver is self-contained, matching the legacy driver’s pattern.

Suggested change
"github.com/Liphium/magic/v3/util"
"github.com/Liphium/magic/v3/util"
_ "github.com/lib/pq"

Copilot uses AI. Check for mistakes.
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.

Postgres Version 18 Support

2 participants