fix: capture ParameterDescription to resolve timestamp OIDs for EXPLAIN by mickamy · Pull Request #25 · mickamy/sql-tap

mickamy · 2026-03-07T01:02:39Z

No description provided.

Copilot

Pull request overview

This PR improves PostgreSQL extended-query parameter decoding by capturing server-resolved parameter OIDs (via ParameterDescription) so binary-encoded timestamps can be decoded into RFC3339 strings (avoiding EXPLAIN failures caused by out-of-range date/time values).

Changes:

Capture Describe + ParameterDescription messages and store authoritative parameter OIDs for statements.
Add test-only exports for decodeBinaryParam and timestamp OID constants.
Add unit tests for DecodeBinaryParam timestamp vs non-timestamp decoding behavior.

Reviewed changes

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

File	Description
`proxy/postgres/conn.go`	Tracks `Describe` and consumes `ParameterDescription` to update parameter OIDs; introduces a mutex for cross-goroutine access to OID state.
`proxy/postgres/export_test.go`	Exposes `decodeBinaryParam` and timestamp OID constants to external tests.
`proxy/postgres/conn_test.go`	Adds unit tests covering `DecodeBinaryParam` behavior for timestamp OIDs and common integer lengths.

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

Copilot · 2026-03-07T01:07:16Z

+	c.lastParamOIDs = m.ParameterOIDs
+	if c.lastDescribeStmt != "" {
+		c.preparedStmtOIDs[c.lastDescribeStmt] = m.ParameterOIDs
+	}


handleParameterDescription always overwrites c.lastParamOIDs, even when the ParameterDescription is for a named prepared statement. Since handleBind uses lastParamOIDs as the fallback for PreparedStatement == "", a ParameterDescription for some other statement can race in and cause unnamed binds to use the wrong OIDs (and thus decode parameters incorrectly). One fix is to only update lastParamOIDs when the described statement name is empty (unnamed statement), and otherwise update only preparedStmtOIDs for the corresponding statement (ideally using an ordered pending-describe queue).

Suggested change

c.lastParamOIDs = m.ParameterOIDs

if c.lastDescribeStmt != "" {

c.preparedStmtOIDs[c.lastDescribeStmt] = m.ParameterOIDs

}

// If lastDescribeStmt is empty, this ParameterDescription applies to the

// unnamed prepared statement; update lastParamOIDs, which is used as the

// fallback for unnamed binds.

if c.lastDescribeStmt == "" {

c.lastParamOIDs = m.ParameterOIDs

return

}

// Otherwise, this ParameterDescription applies to a named prepared

// statement; record its OIDs in preparedStmtOIDs without touching

// lastParamOIDs (which should remain associated with the unnamed stmt).

c.preparedStmtOIDs[c.lastDescribeStmt] = m.ParameterOIDs

Copilot · 2026-03-07T01:07:16Z

+	OidTimestamp   = oidTimestamp
+	OidTimestampTZ = oidTimestampTZ


Exported identifiers for initialisms are typically all-caps in this codebase (e.g., TxID, preparedStmtOIDs). For consistency, consider renaming OidTimestamp / OidTimestampTZ to OIDTimestamp / OIDTimestampTZ (and updating the tests accordingly).

Suggested change

OidTimestamp = oidTimestamp

OidTimestampTZ = oidTimestampTZ

OIDTimestamp = oidTimestamp

OIDTimestampTZ = oidTimestampTZ

Copilot · 2026-03-07T01:07:17Z

+func (c *conn) handleDescribe(m *pgproto.Describe) {
+	if m.ObjectType == 'S' {
+		c.stmtMu.Lock()
+		c.lastDescribeStmt = m.Name
+		c.stmtMu.Unlock()


lastDescribeStmt stores only a single statement name, but ParameterDescription responses arrive asynchronously from the upstream goroutine and PostgreSQL allows pipelining multiple Describe('S') messages. If multiple Describe(Statement) messages are in-flight, lastDescribeStmt can be overwritten before the first ParameterDescription arrives, causing OIDs to be associated with the wrong prepared statement. Consider tracking a FIFO queue of pending described statement names (push in handleDescribe, pop in handleParameterDescription) so responses are matched to the correct statement.

…oid polluting unnamed OIDs

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

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

Copilot · 2026-03-07T01:16:36Z

+func (c *conn) handleDescribe(m *pgproto.Describe) {
+	if m.ObjectType == 'S' {
+		c.stmtMu.Lock()
+		c.pendingDescribes = append(c.pendingDescribes, m.Name)
+		c.stmtMu.Unlock()
+	}
+}
+
+// handleParameterDescription captures the server-resolved parameter OIDs
+// returned by the upstream in response to a Describe(Statement) message.
+// These OIDs are authoritative — they override the OIDs from Parse, which
+// are often all zeros (meaning "let the server decide").
+// Responses arrive in the same order as the corresponding Describe requests,
+// so we pop from the front of pendingDescribes to match them.
+func (c *conn) handleParameterDescription(m *pgproto.ParameterDescription) {
+	c.stmtMu.Lock()
+	defer c.stmtMu.Unlock()
+
+	if len(c.pendingDescribes) == 0 {
+		return
+	}
+	name := c.pendingDescribes[0]
+	c.pendingDescribes = c.pendingDescribes[1:]
+
+	if name == "" {
+		// Unnamed statement: update the fallback OIDs used by unnamed binds.
+		c.lastParamOIDs = m.ParameterOIDs
+	} else {
+		// Named statement: only update its entry without touching lastParamOIDs.
+		c.preparedStmtOIDs[name] = m.ParameterOIDs
+	}


pendingDescribes is only advanced when a ParameterDescription arrives. If the upstream returns an ErrorResponse for a Describe('S') (e.g., unknown statement), there will be no ParameterDescription, leaving a stale entry in the FIFO and causing the next ParameterDescription to be associated with the wrong statement. Consider clearing (or popping) pending describe entries on relevant ErrorResponse/sync boundaries (e.g., in handleErrorResponse under stmtMu) to keep the queue in sync and avoid mis-decoding subsequent bind params.

Copilot · 2026-03-07T01:16:36Z

+// handleParameterDescription captures the server-resolved parameter OIDs
+// returned by the upstream in response to a Describe(Statement) message.
+// These OIDs are authoritative — they override the OIDs from Parse, which
+// are often all zeros (meaning "let the server decide").
+// Responses arrive in the same order as the corresponding Describe requests,
+// so we pop from the front of pendingDescribes to match them.
+func (c *conn) handleParameterDescription(m *pgproto.ParameterDescription) {
+	c.stmtMu.Lock()
+	defer c.stmtMu.Unlock()
+
+	if len(c.pendingDescribes) == 0 {
+		return
+	}
+	name := c.pendingDescribes[0]
+	c.pendingDescribes = c.pendingDescribes[1:]
+
+	if name == "" {
+		// Unnamed statement: update the fallback OIDs used by unnamed binds.
+		c.lastParamOIDs = m.ParameterOIDs
+	} else {
+		// Named statement: only update its entry without touching lastParamOIDs.
+		c.preparedStmtOIDs[name] = m.ParameterOIDs
+	}


New behavior captures server-resolved parameter OIDs via Describe/ParameterDescription, but there are no tests asserting that (a) unnamed binds pick up ParameterDescription OIDs and (b) named prepared statements override per-statement OIDs. Adding a small protocol-level/unit test around handleDescribe + handleParameterDescription + handleBind would prevent regressions where timestamps fall back to OID=0 and decode incorrectly.

Copilot · 2026-03-07T01:16:37Z

    branches:
      - "**"
    tags:
      - "v*"


CI no longer runs for pull requests because the pull_request trigger was removed and there is no other workflow in .github/workflows/ that triggers on PRs. This will prevent required checks from running before merge. Re-add a pull_request trigger (or document/implement an alternative PR validation workflow) to keep PR validation in place.

Suggested change

- "v*"

- "v*"

pull_request:

branches:

- "**"

Copilot · 2026-03-07T01:16:37Z

      - uses: actions/setup-go@v5
        with:
-          go-version: "1.25"
+          go-version: 1.26

      - name: golangci-lint
        uses: golangci/golangci-lint-action@v9

  test:
    runs-on: ubuntu-latest
-    strategy:
-      matrix:
-        go-version: [ "1.25", "1.26" ]
    steps:
      - uses: actions/checkout@v6

      - uses: actions/setup-go@v5
        with:
-          go-version: ${{ matrix.go-version }}
+          go-version: 1.26


In GitHub Actions YAML, unquoted values like go-version: 1.26 are parsed as numbers. Quoting the version (e.g., "1.26") avoids edge cases where YAML numeric parsing can change the value (notably for versions like 1.10) and matches the convention used in release.yml.

Copilot · 2026-03-07T01:16:37Z

 module github.com/mickamy/sql-tap

-go 1.25.0
+go 1.26.1


The PR title is specific to PostgreSQL ParameterDescription handling, but this change also bumps the module Go version (and separately modifies CI/docs). This makes the PR harder to review and can complicate rollback. Consider splitting the Go/tooling/docs changes into separate PR(s) unless they are required for the protocol fix.

…tamination

…s thread safety

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

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

Copilot · 2026-03-07T01:43:06Z

 		_ = f.Close()
-		_ = os.Remove(path) //nolint:gosec // path is a controlled temp file created by this function
+		_ = os.Remove(path)
 		return func() tea.Msg {


Removing the previous //nolint:gosec here will likely cause gosec G304-style findings for using a variable file path. Since path comes from os.CreateTemp, it’s safe, but with gosec enabled this may fail CI; consider restoring the suppression with a brief rationale.

Copilot · 2026-03-07T01:43:07Z


 	p := pproxy.New(addr, upstream)
-	ctx, cancel := context.WithCancel(t.Context())
+	ctx, cancel := context.WithCancel(t.Context()) //nolint:gosec // cancel is deferred below via t.Cleanup


The inline suppression uses //nolint:gosec, but the warning for context.WithCancel without an immediate cancel() is typically from govet's lostcancel analyzer, not gosec. As written, this comment likely won’t suppress the CI finding; use the correct nolint tag (e.g. lostcancel/govet) or adjust the code in a way the analyzer recognizes.

Suggested change

ctx, cancel := context.WithCancel(t.Context()) //nolint:gosec // cancel is deferred below via t.Cleanup

ctx, cancel := context.WithCancel(t.Context()) //nolint:govet,lostcancel // cancel is deferred below via t.Cleanup

Copilot · 2026-03-07T01:43:07Z


 	p := mproxy.New(addr, upstream)
-	ctx, cancel := context.WithCancel(t.Context())
+	ctx, cancel := context.WithCancel(t.Context()) //nolint:gosec // cancel is deferred below via t.Cleanup


The inline suppression uses //nolint:gosec, but the warning for context.WithCancel without an immediate cancel() is typically from govet's lostcancel analyzer, not gosec. As written, this comment likely won’t suppress the CI finding; use the correct nolint tag (e.g. lostcancel/govet) or adjust the code in a way the analyzer recognizes.

Suggested change

ctx, cancel := context.WithCancel(t.Context()) //nolint:gosec // cancel is deferred below via t.Cleanup

ctx, cancel := context.WithCancel(t.Context()) //nolint:govet // cancel is deferred below via t.Cleanup

Copilot · 2026-03-07T01:43:07Z

 	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, ts.URL+"/", nil)
-	resp, err := http.DefaultClient.Do(req) //nolint:gosec // test URL
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {


Removing the //nolint:gosec suppression here will likely reintroduce gosec G107 (variable URL in HTTP request). Since gosec is enabled in .golangci.yaml, this may fail CI; consider restoring the suppression (or using an alternative suppression form like #nosec G107) with an appropriate rationale for this test-only request.

Copilot · 2026-03-07T01:43:07Z

 	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, ts.URL+"/api/events", nil)
-	resp, err := http.DefaultClient.Do(req) //nolint:gosec // test URL
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {


Removing the //nolint:gosec suppression here will likely reintroduce gosec G107 (variable URL in HTTP request). Since gosec is enabled in .golangci.yaml, this may fail CI; consider restoring the suppression (or using #nosec G107) with a short test-only justification.

Copilot · 2026-03-07T01:43:07Z

 	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, ts.URL+"/api/events", nil)
-	resp, err := http.DefaultClient.Do(req) //nolint:gosec // test URL
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {


Removing the //nolint:gosec suppression here will likely reintroduce gosec G107 (variable URL in HTTP request). Since gosec is enabled in .golangci.yaml, this may fail CI; consider restoring the suppression (or using #nosec G107) with a short test-only justification.

Copilot · 2026-03-07T01:43:08Z

 	req, _ := http.NewRequestWithContext(ctx, http.MethodPost, ts.URL+"/api/explain", body)
 	req.Header.Set("Content-Type", "application/json")
-	resp, err := http.DefaultClient.Do(req) //nolint:gosec // test URL
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {


Removing the //nolint:gosec suppression here will likely reintroduce gosec G107 (variable URL in HTTP request). Since gosec is enabled in .golangci.yaml, this may fail CI; consider restoring the suppression (or using #nosec G107) with a short test-only justification.

fix: capture ParameterDescription to resolve timestamp OIDs for EXPLAIN

99b7c20

Copilot AI review requested due to automatic review settings March 7, 2026 01:02

Copilot started reviewing on behalf of mickamy March 7, 2026 01:03 View session

mickamy added 3 commits March 7, 2026 10:05

chore: use golang 1.26.1

a0f8e19

docs: standalone sql-tap image

1ab151e

ci: do not run CI workflow on pull request

147ca42

Copilot AI reviewed Mar 7, 2026

View reviewed changes

mickamy added 2 commits March 7, 2026 10:08

lint: make linter happy

3a55269

fix: use FIFO queue for Describe→ParameterDescription matching and av…

6e43e3d

…oid polluting unnamed OIDs

Copilot AI review requested due to automatic review settings March 7, 2026 01:12

Copilot started reviewing on behalf of mickamy March 7, 2026 01:13 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

mickamy added 6 commits March 7, 2026 10:25

fix: drain pending describes on ReadyForQuery and avoid OID cross-con…

e6682d9

…tamination

lint: make linter happy

709ef51

fix: release pendingDescribes backing array and document preparedStmt…

e53db2f

…s thread safety

ci: quote go-version to prevent YAML numeric parsing

c2c23f7

ci: add pull_request trigger for fork PR validation

f56daf3

test: add unit tests for Describe→ParameterDescription→Bind OID flow

631015d

Copilot AI review requested due to automatic review settings March 7, 2026 01:36

Copilot started reviewing on behalf of mickamy March 7, 2026 01:37 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

mickamy merged commit 49ccb34 into main Mar 7, 2026
9 checks passed

mickamy deleted the fix/timestamp branch March 7, 2026 01:45

-	c.lastParamOIDs = m.ParameterOIDs
-	if c.lastDescribeStmt != "" {
-		c.preparedStmtOIDs[c.lastDescribeStmt] = m.ParameterOIDs
-	}
+	// If lastDescribeStmt is empty, this ParameterDescription applies to the
+	// unnamed prepared statement; update lastParamOIDs, which is used as the
+	// fallback for unnamed binds.
+	if c.lastDescribeStmt == "" {
+		c.lastParamOIDs = m.ParameterOIDs
+		return
+	}
+	// Otherwise, this ParameterDescription applies to a named prepared
+	// statement; record its OIDs in preparedStmtOIDs without touching
+	// lastParamOIDs (which should remain associated with the unnamed stmt).
+	c.preparedStmtOIDs[c.lastDescribeStmt] = m.ParameterOIDs

	ctx, cancel := context.WithCancel(t.Context()) //nolint:gosec // cancel is deferred below via t.Cleanup
	ctx, cancel := context.WithCancel(t.Context()) //nolint:govet,lostcancel // cancel is deferred below via t.Cleanup

Uh oh!

Conversation

mickamy commented Mar 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants