Skip to content

fix: use dedicated connection in ApplySchema to preserve search_path#375

Merged
tianzhou merged 1 commit intomainfrom
fix/pre-existing-test-failures
Mar 26, 2026
Merged

fix: use dedicated connection in ApplySchema to preserve search_path#375
tianzhou merged 1 commit intomainfrom
fix/pre-existing-test-failures

Conversation

@tianzhou
Copy link
Contributor

Summary

  • ApplySchema (both embedded and external) used separate ExecContext calls on *sql.DB (connection pool) for SET search_path and SQL execution. Go's database/sql does not guarantee the same connection across calls, so session-scoped settings like search_path could silently be lost, causing objects to be created in the wrong schema.
  • Fix: acquire a single *sql.Conn from the pool via db.Conn(ctx) and use it for all statements, guaranteeing SET search_path affects subsequent SQL execution.
  • Updated ExecContextWithLogging to accept an execer interface so it works with both *sql.DB and *sql.Conn.

Test plan

  • TestPlanAndApply/create_table_add_column_cross_schema_custom_type — previously failing, now passes
  • TestPlanAndApply/dependency_issue_300_function_table_composite_type — previously failing, now passes
  • Full test suite (go test ./...) passes

🤖 Generated with Claude Code

ApplySchema used separate ExecContext calls on *sql.DB (connection pool)
for SET search_path and SQL execution. Go's database/sql does not
guarantee the same connection across calls, so session-scoped settings
like search_path could be lost, causing objects to be created in the
wrong schema.

Fix: acquire a single *sql.Conn from the pool and use it for all
statements, guaranteeing SET search_path affects subsequent SQL.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 26, 2026 07:52
@greptile-apps
Copy link

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR fixes a real concurrency/session-state bug in ApplySchema for both the embedded and external Postgres providers. Previously, SET search_path and the subsequent DDL execution were issued as independent ExecContext calls on *sql.DB (a connection pool). Go's database/sql makes no guarantee that consecutive calls use the same physical connection, meaning the session-scoped search_path could silently be lost, causing objects to be created in the wrong schema. The fix is minimal and correct: a single *sql.Conn is acquired at the top of each ApplySchema and all statements are routed through it, and the ExecContextWithLogging helper is generalised to accept an execer interface satisfied by both *sql.DB and *sql.Conn.\n\nKey changes:\n- cmd/util/sql_logger.go: Adds unexported execer interface; updates ExecContextWithLogging signature to execer — no breaking change for callers.\n- internal/postgres/embedded.go and internal/postgres/external.go: Acquire *sql.Conn via db.Conn(ctx) at the start of ApplySchema with defer conn.Close(), and route all four statement executions through the dedicated connection.\n- Stop() cleanup methods in both files correctly continue using *sql.DB directly, since they reference the schema by explicit name and do not rely on search_path.

Confidence Score: 5/5

Safe to merge — the fix is minimal, targeted, and correctly addresses a real session-isolation bug with no regressions to the surrounding cleanup paths.

The change is small (three files), logically sound, and covers both affected code paths (embedded and external) symmetrically. The interface generalisation in sql_logger.go is backward-compatible since Go uses structural typing and existing callers passing *sql.DB continue to compile unchanged. defer conn.Close() correctly handles both success and early-return error paths. The PR description references two previously failing integration tests that now pass, and reports a clean full test suite run.

No files require special attention.

Important Files Changed

Filename Overview
cmd/util/sql_logger.go Introduces unexported execer interface and updates ExecContextWithLogging signature from *sql.DB to execer, correctly enabling dual use with *sql.DB and *sql.Conn without any breaking changes to callers.
internal/postgres/embedded.go Acquires a single *sql.Conn at the start of ApplySchema and routes all statements through it, guaranteeing SET search_path is visible to subsequent DDL; connection is properly released via defer conn.Close().
internal/postgres/external.go Mirrors the same db.Conn(ctx) / defer conn.Close() pattern applied to EmbeddedPostgres, consistently fixing the search_path race for the external database path as well.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant ApplySchema
    participant sql.DB (pool)
    participant sql.Conn (dedicated)
    participant PostgreSQL

    Caller->>ApplySchema: ApplySchema(ctx, schema, sql)
    ApplySchema->>sql.DB (pool): db.Conn(ctx)
    sql.DB (pool)-->>ApplySchema: conn (*sql.Conn)

    ApplySchema->>sql.Conn (dedicated): DROP SCHEMA IF EXISTS tempSchema CASCADE
    sql.Conn (dedicated)->>PostgreSQL: execute
    PostgreSQL-->>sql.Conn (dedicated): OK

    ApplySchema->>sql.Conn (dedicated): CREATE SCHEMA tempSchema
    sql.Conn (dedicated)->>PostgreSQL: execute
    PostgreSQL-->>sql.Conn (dedicated): OK

    ApplySchema->>sql.Conn (dedicated): SET search_path TO tempSchema, public
    sql.Conn (dedicated)->>PostgreSQL: execute (session-scoped to this conn)
    PostgreSQL-->>sql.Conn (dedicated): OK

    ApplySchema->>sql.Conn (dedicated): [schema DDL — relies on search_path]
    sql.Conn (dedicated)->>PostgreSQL: execute (same session, search_path active)
    PostgreSQL-->>sql.Conn (dedicated): OK

    ApplySchema->>sql.Conn (dedicated): conn.Close() (defer)
    sql.Conn (dedicated)->>sql.DB (pool): return to pool
    ApplySchema-->>Caller: nil
Loading

Reviews (1): Last reviewed commit: "fix: use dedicated connection in ApplySc..." | Re-trigger Greptile

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

Ensures ApplySchema reliably applies desired-state SQL under the intended search_path by executing SET search_path and subsequent DDL on the same physical PostgreSQL connection, avoiding database/sql pool connection switching.

Changes:

  • Updated embedded and external ApplySchema implementations to use a dedicated *sql.Conn for all statements (including SET search_path).
  • Generalized ExecContextWithLogging to accept an execer interface so it can execute via either *sql.DB or *sql.Conn.

Reviewed changes

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

File Description
internal/postgres/external.go Uses a dedicated *sql.Conn in ExternalDatabase.ApplySchema so search_path is preserved across statements.
internal/postgres/embedded.go Uses a dedicated *sql.Conn in EmbeddedPostgres.ApplySchema so search_path is preserved across statements.
cmd/util/sql_logger.go Refactors ExecContextWithLogging to accept an execer interface for compatibility with *sql.Conn.

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

@tianzhou tianzhou merged commit 03a4a86 into main Mar 26, 2026
6 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.

2 participants