fix: show error location with SQL context when plan fails by tianzhou · Pull Request #394 · pgplex/pgschema

tianzhou · 2026-04-08T07:02:57Z

Summary

When applying desired state SQL fails during plan, extract the character position from PostgreSQL's error response (pgconn.PgError.Position) and show a context snippet with line numbers around the error location
Previously the error was generic (e.g., syntax error at or near "SELECT"), making it nearly impossible to locate the issue in large schema files
Handles multi-byte UTF-8 correctly (PostgreSQL positions count characters, not bytes)

Before:

Error: failed to apply desired state: failed to apply schema SQL to temporary schema pgschema_tmp_...: ERROR: syntax error at or near "SELECT" (SQLSTATE 42601)

After:

Error: failed to apply desired state: failed to apply schema SQL to temporary schema pgschema_tmp_...: ERROR: syntax error at or near "SELECT" (SQLSTATE 42601)

Error location (line 5, column 1):
      2 | CREATE TABLE bar (
      3 |   name text
      4 | );
  >   5 | SELECT 1;
      6 | CREATE TABLE baz (id int);

Closes #389

Test plan

Unit tests for enhanceApplyError (PgError with position, without position, non-PgError passthrough)
All existing internal/postgres tests pass

🤖 Generated with Claude Code

… fails When applying schema SQL fails during plan, PostgreSQL returns an error with a character position but pgschema discarded it, producing generic messages like "syntax error at or near SELECT". Now extracts the position from pgconn.PgError and shows a context snippet with line numbers, making it easy to locate the problematic statement. Closes #389 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR improves plan-stage error reporting when applying desired-state SQL fails by extracting PostgreSQL error positions and printing a numbered SQL context snippet around the failure location.

Changes:

Wrap schema-apply errors with an enhanced message that includes error line/column and surrounding SQL context.
Add enhanceApplyError to parse pgconn.PgError.Position using rune offsets to support UTF-8 character positions.
Add unit tests covering PgError-with-position behavior and passthrough behavior.

Reviewed changes

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

File	Description
internal/postgres/external.go	Wrap apply-schema failures with `enhanceApplyError(...)` to include SQL context.
internal/postgres/embedded.go	Same enhancement for embedded Postgres plan execution.
internal/postgres/desired_state.go	Add `enhanceApplyError` that computes line/column from `PgError.Position` and formats a context snippet.
internal/postgres/desired_state_test.go	Add unit tests for the new error enhancement behavior.

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

internal/postgres/desired_state_test.go

…hrough - Add test case with multi-byte character (café) to verify character-based position handling works correctly - Use identity comparison (result != origErr) instead of string comparison for passthrough assertions to ensure no accidental wrapping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

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

Copilot · 2026-04-08T07:20:14Z

internal/postgres/desired_state.go

+	const contextLines = 3
+	lines := strings.Split(sql, "\n")
+
+	startLine := max(line-contextLines, 1)
+	endLine := min(line+contextLines, len(lines))


strings.Split(sql, "\n") will leave \r characters intact when the input SQL uses CRLF line endings. Those \r characters will be embedded into the rendered context snippet and can cause messy/overwritten terminal output on Windows or when schema files are checked out with CRLF. Consider normalizing line endings (e.g., replace \r\n with \n) or trimming a trailing \r from each displayed line before writing the snippet.

The SQL here comes from PostgreSQL's wire protocol or pgschema's own dump output — neither produces CRLF. Even if a user's schema file has CRLF line endings, Go reads and sends it as-is, and the Position from PostgreSQL will be consistent with whatever was sent. Adding CRLF normalization for a diagnostic error path that won't encounter it in practice isn't worth the complexity.

Copilot · 2026-04-08T07:20:15Z

internal/postgres/desired_state.go

+		snippet.WriteString(fmt.Sprintf("%s%5d | %s\n", prefix, i, lines[i-1]))
+	}
+
+	return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())


The snippet builder always appends a trailing newline (and snippet.String() is then appended verbatim). This makes the final error string end with an extra newline, which can produce an extra blank line in CLI output and makes error strings harder to compare in tests/logging. Consider avoiding the final \n (e.g., only add newlines between lines, or strings.TrimRight(snippet.String(), "\n") before formatting).

Suggested change

return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())

return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, strings.TrimRight(snippet.String(), "\n"))

The trailing newline is intentional — it ensures the snippet block ends cleanly before the shell prompt or any subsequent output. This is CLI diagnostic output, not a string used in test comparisons.

greptile-apps · 2026-04-08T07:35:48Z

Greptile Summary

This PR adds enhanceApplyError to extract PostgreSQL's PgError.Position (a 1-based character offset) and display a ±3-line SQL snippet with line/column context when plan's desired-state SQL application fails. Both the embedded and external database paths are updated symmetrically. The implementation is correct: rune-aware position calculation, proper pass-through for non-PgError or zero-position cases, and the snippet is generated from the same schemaAgnosticSQL string sent to PostgreSQL, keeping positions consistent.

Confidence Score: 5/5

Safe to merge — the feature is purely additive, all remaining findings are P2 style suggestions.

The core logic is correct: rune-aware position arithmetic, safe bounds checking, proper error-chain wrapping with %w, and symmetric application to both database backends. The two P2 comments (byte-vs-rune in one test assertion and a trailing newline in the error string) are cosmetic and do not affect correctness or reliability.

No files require special attention.

Vulnerabilities

No security concerns identified. The error enhancement reads from the existing SQL string and error object only — no user input is evaluated or executed, and no new attack surface is introduced.

Important Files Changed

Filename	Overview
internal/postgres/desired_state.go	Adds `enhanceApplyError` helper: correctly handles non-PgError pass-through, zero-position guard, rune-aware line/column calculation, and bounded context window around the error line.
internal/postgres/desired_state_test.go	Good unit test coverage (PgError with position, multi-byte UTF-8, non-PgError, zero-position pass-through); the ASCII test case uses `strings.Index` (byte offset) to fake a character position, which works but is subtly incorrect in general.
internal/postgres/embedded.go	One-line change: wraps the ApplySchema error with enhanceApplyError(err, schemaAgnosticSQL) — position and snippet are both relative to the same schemaAgnosticSQL, so they are consistent.
internal/postgres/external.go	Symmetric one-line change mirroring embedded.go; correctly applies enhanceApplyError to the external database path as well.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ApplySchema called with user SQL] --> B[stripSchemaQualifications / replaceSchema*]
    B --> C[schemaAgnosticSQL]
    C --> D[ExecContextWithLogging]
    D -->|success| E[return nil]
    D -->|error| F[enhanceApplyError err, schemaAgnosticSQL]
    F --> G{errors.As pgErr AND Position != 0?}
    G -->|No| H[return err unchanged]
    G -->|Yes| I[Convert Position to 0-based rune index]
    I --> J[Walk runes to compute line + lineStart]
    J --> K[col = pos - lineStart + 1]
    K --> L[Split SQL into lines]
    L --> M[Build ±3-line snippet with > marker]
    M --> N[fmt.Errorf with %w + location context]
    N --> O[return enhanced error]
    O --> P[fmt.Errorf: failed to apply schema SQL ...]

_{Reviews (1): Last reviewed commit: "test: add multi-byte UTF-8 test and use ..." | Re-trigger Greptile}

greptile-apps · 2026-04-08T07:35:51Z

internal/postgres/desired_state_test.go

+
+	t.Run("pgError with position", func(t *testing.T) {
+		// Position points to "SELECT" on line 5
+		pos := int32(strings.Index(sql, "SELECT 1") + 1) // 1-based


Byte offset used as character position

strings.Index returns a byte offset, but pgconn.PgError.Position is a character (rune) offset — the two happen to be equal here only because the test SQL is pure ASCII. Consider using utf8.RuneCountInString(sql[:strings.Index(sql, "SELECT 1")]) + 1 to make the intent explicit and future-proof against non-ASCII test data.

Suggested change

pos := int32(strings.Index(sql, "SELECT 1") + 1) // 1-based

pos := int32(utf8.RuneCountInString(sql[:strings.Index(sql, "SELECT 1")]) + 1) // 1-based character offset

greptile-apps · 2026-04-08T07:35:52Z

internal/postgres/desired_state.go

+		snippet.WriteString(fmt.Sprintf("%s%5d | %s\n", prefix, i, lines[i-1]))
+	}
+
+	return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())


Trailing newline in error message

snippet.String() already ends with \n (every line in the loop appends one), so the returned error message ends with a bare newline. This can produce double-blank-lines or extra whitespace when callers wrap the error with fmt.Errorf("...: %w", enhancedErr). A simple strings.TrimRight(snippet.String(), "\n") keeps the snippet clean.

Suggested change

return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())

return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, strings.TrimRight(snippet.String(), "\n"))

Copilot AI review requested due to automatic review settings April 8, 2026 07:02

Copilot started reviewing on behalf of tianzhou April 8, 2026 07:03 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

internal/postgres/desired_state_test.go Show resolved Hide resolved

internal/postgres/desired_state_test.go Show resolved Hide resolved

internal/postgres/desired_state_test.go Show resolved Hide resolved

tianzhou requested a review from Copilot April 8, 2026 07:15

Copilot started reviewing on behalf of tianzhou April 8, 2026 07:16 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

tianzhou merged commit e1cffc7 into main Apr 8, 2026
5 checks passed

greptile-apps bot reviewed Apr 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: show error location with SQL context when plan fails#394

fix: show error location with SQL context when plan fails#394
tianzhou merged 2 commits intomainfrom
improve-error-location-reporting

tianzhou commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

tianzhou Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

tianzhou Apr 8, 2026

Uh oh!

Uh oh!

greptile-apps bot commented Apr 8, 2026

Vulnerabilities

Uh oh!

greptile-apps bot Apr 8, 2026

Uh oh!

greptile-apps bot Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())
	return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, strings.TrimRight(snippet.String(), "\n"))

	pos := int32(strings.Index(sql, "SELECT 1") + 1) // 1-based
	pos := int32(utf8.RuneCountInString(sql[:strings.Index(sql, "SELECT 1")]) + 1) // 1-based character offset

Conversation

tianzhou commented Apr 8, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

tianzhou Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

tianzhou Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot commented Apr 8, 2026

Greptile Summary

Confidence Score: 5/5

Vulnerabilities

Important Files Changed

Flowchart

Uh oh!

greptile-apps bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants