Skip to content

Experimental postgres query (PR 4/4): cancellation, timeout, TUI#5143

Open
simonfaltum wants to merge 9 commits intosimonfaltum/postgres-query-pr3-multi-inputfrom
simonfaltum/postgres-query-pr4-cancel
Open

Experimental postgres query (PR 4/4): cancellation, timeout, TUI#5143
simonfaltum wants to merge 9 commits intosimonfaltum/postgres-query-pr3-multi-inputfrom
simonfaltum/postgres-query-pr4-cancel

Conversation

@simonfaltum
Copy link
Copy Markdown
Member

@simonfaltum simonfaltum commented Apr 30, 2026 •
edited
Loading

PR Stack

  1. #5135 — PR 1: scaffold + autoscaling targeting + text output
  2. #5136 — PR 2: provisioned + JSON/CSV streaming + types
  3. #5138 — PR 3: multi-input + multi-statement rejection + error formatting
  4. PR 4 (this PR) — #5143 — cancellation + timeout + TUI for >30 rows

Stacked on PR 3.

Why

PR 3 finished the input ergonomics. The remaining commitments before this command earns the "experimental" label:

  • A long SELECT shouldn't survive Ctrl+C. Today the pgx default closes the TCP socket but leaves the server-side query running.
  • CI scripts want --timeout so a single statement can't pin a runner.
  • Interactive users with >30 rows benefit from a scrollable browser instead of a wall of text.

Changes

Before: Ctrl+C tears down TCP but the query runs to completion server-side. --timeout doesn't exist. >30 rows scroll past in the terminal.

Now: Ctrl+C cancels the in-flight query at the server. --timeout 30s enforces a per-statement deadline. >30 rows on a TTY open the libs/tableview viewer.

Specifically:

  • Cancellation watcher. buildPgxConfig now installs CancelRequestContextWatcherHandler with CancelRequestDelay=0, DeadlineDelay=5s. Zero DeadlineDelay would race the cancel-request and could leave the connection unusable; 5s gives the cancel round-trip time to land.
  • Signal handler. Per-invocation goroutine watches SIGINT and SIGTERM. Calls cancel; defer'd stop drains the channel.
  • --timeout flag. Per-statement context.WithTimeout child of the signal-scoped ctx. reportCancellation distinguishes Ctrl+C ("Query cancelled."), timeout ("Query timed out after Xs."), and plain query errors. Returns (msg, invocationScoped) so the multi-input loop can drop the source prefix on user-cancel.
  • TUI for >30 rows. textSink now has an interactive mode; runQuery enables it when stdout is a prompt-capable TTY. Static tabwriter table for small results and pipes; libs/tableview for big interactive ones. If tableview.Run fails (TUI startup, terminal resize race) the sink falls through to the static tabwriter path so the user still sees the rows.

Integration tests aren't included: aitools (the other experimental command) doesn't have them either. Acceptance + unit tests cover argument validation, targeting resolution (SDK-mocked), and output shapes; cancellation/timeout are unit-tested via the seam in cancel_test.go. Real-wire integration tests are the right addition when this command graduates from experimental.

Test plan

  • go test ./experimental/postgres/... (cancel/timeout/signal helpers, race-precedence pinning)
  • go test ./acceptance -run TestAccept/cmd/(psql|experimental/postgres) (no regressions)
  • go tool ... golangci-lint run ./experimental/postgres/... (0 issues)
  • Manual: Ctrl+C during SELECT pg_sleep(60) cancels the server-side query within ~5s.

@simonfaltum
Cancellation watcher + --timeout + TUI for >30 rows + integration tests
d0fe9b8 
This is PR 4 of the experimental postgres query stack and finishes the
plan's v1 scope.

Cancellation watcher: pgx.ConnConfig now installs a
CancelRequestContextWatcherHandler with CancelRequestDelay=0 (send the
cancel-request immediately on ctx cancel) and DeadlineDelay=5s (fall back
to deadlining the connection if the cancel-request hasn't terminated the
query within 5s). Without this, Ctrl+C tears down the TCP connection but
leaves the server-side query running until it next writes.

Signal handling: a per-invocation signal goroutine watches SIGINT and
SIGTERM, cancelling the connection-scoped ctx. The defer'd stop drains
the signal channel so a queued signal during shutdown does not leak. On
Windows, Go's console-control-handler routes Ctrl+C to os.Interrupt, so
the same code path covers the Windows runner.

--timeout: per-statement deadline applied via context.WithTimeout. A
fresh deadline starts for each input unit; the connection-scoped ctx
remains the parent so a SIGINT during unit N immediately cancels both.
reportCancellation distinguishes the three error sources (ctx.Canceled,
ctx.DeadlineExceeded, plain pg error) so the user-visible message is
"Query cancelled.", "Query timed out after Xs.", or the formatted pg
error respectively.

TUI for >30 rows: when --output text and stdout is a prompt-capable
TTY, results larger than staticTableThreshold (=30, matching aitools)
hand off to libs/tableview's interactive viewer. Smaller results stay
in the static tabwriter path so non-interactive callers see no change.

Integration tests live in integration/cmd/postgres/. Skipped unless
DATABRICKS_POSTGRES_INTEGRATION_TARGET is set; covers single-input
JSON, command-only, --timeout firing, multi-input JSON, and a CSV
streaming smoke test (generate_series(1, 100)). Ctrl+C is documented
as needing a separate harness because it requires a child process.

Co-authored-by: Isaac
This was referenced Apr 30, 2026
Open
Open
@simonfaltum
Address PR 4 review feedback round 1
43b2667 
SHOULDs:
- signals.go: drop the false "drains the signal channel" claim from the
  doc comment. signal.Stop blocks future deliveries; the 1-buffered
  channel is GC'd on return so no explicit drain is needed.
- integration test: drop the dead `_ "cmd/experimental"` blank import
  (testcli.NewRunner already pulls cmd/experimental in transitively).
- integration test: delete the cancel-on-interrupt stub; documented as
  a follow-up because Ctrl+C testing requires a child-process harness
  that's outside the scope of this PR.
- query.go: when an invocation-scoped error fires (Ctrl+C, --timeout)
  in multi-input mode, drop the `<source>:` prefix. The user knows which
  invocation they cancelled; "--file foo.sql: Query cancelled." reads
  worse than "Query cancelled." reportCancellation now returns
  (msg, invocationScoped) so the caller picks the right shape.
- withStatementTimeout: trim the v2-speculation from the doc comment.

CONSIDERs:
- C2: rename watchInterruptSignals's stop closure semantics to
  acknowledge it cancels the parent ctx as a side effect.
- C4: TestReportCancellation_BothFire_CancelWinsRace pins the
  precedence (user cancel beats coincidental deadline).
- C6: drop the redundant require.Error after RequireErrorRun (which
  already calls require.Error internally).

Plus integration test polish:
- Parse JSON outputs instead of substring-matching so encoder drift
  doesn't break tests.
- Tighten timeout assertion from <5s to <3s so a regression to
  TCP-keepalive timeout (~minutes) would show.
- Bump generate_series bound from 100 to 100k so streaming actually
  exercises memory pressure.

Co-authored-by: Isaac
@simonfaltum
PR 4 r2: warm-up TimeoutFires test before timing --timeout
c57805f 
Round-2 reviewer flagged that the previous <3s bound was tight enough
to flake on a cold Lakebase autoscaling endpoint, where auth + connect
+ cold-start can plausibly take >2s on its own. The regression we
actually want to catch (silent fall-back to TCP keepalive) takes
minutes, so <5s is enough.

Add a warm-up RequireSuccessfulRun before timing so the assertion
measures what it claims to measure: how long the 1-second deadline
takes to actually cancel the in-flight statement.

Co-authored-by: Isaac
@simonfaltum
Drop integration tests for experimental command
6cd264e 
Other experimental commands (aitools) have no integration tests; an
experimental command is by definition pre-stabilization, and gating
its real-wire test on a custom env var introduces friction without a
clear win. Acceptance tests + unit tests already cover argument
validation, targeting resolution (SDK-mocked), and the streaming /
multi-input output shapes. The cancellation watcher and --timeout are
unit-tested via the seam in cancel_test.go.

When this command graduates from experimental, integration tests are
the right addition; for v1 they were over-engineered.

Co-authored-by: Isaac
@simonfaltum simonfaltum changed the title Experimental postgres query (PR 4/4): cancellation, timeout, TUI, integration tests Experimental postgres query (PR 4/4): cancellation, timeout, TUI Apr 30, 2026
@simonfaltum simonfaltum mentioned this pull request Apr 30, 2026
Open
4 tasks
@simonfaltum
Merge branch 'simonfaltum/postgres-query-pr3-multi-input' into simonf…
5dbb846 
…altum/postgres-query-pr4-cancel

# Conflicts:
#	experimental/postgres/cmd/query.go
@arsenyinfo
Copy link
Copy Markdown
Contributor

arsenyinfo commented Apr 30, 2026

JSON duplicate column name collision produces duplicate keys in output

  • Priority: P2
  • Location: experimental/postgres/cmd/render_json.go:57-65
  • Scenario: A query returns columns ["id", "id__2", "id"]. The deduplication logic renames the second id to id__2 (via fmt.Sprintf("%s__%d", f.Name, seen[name])) without checking whether id__2 is already assigned. Row() then emits keys from s.columns verbatim, producing a JSON object with two id__2 keys. Reachable with joins or computed-column aliases.
  • Potential solution: After generating a candidate renamed column, check it against already-assigned entries in s.columns (or a secondary set); keep incrementing the suffix (id__3, id__4, …) until the candidate is unique.

No graceful degradation when tableview.Run fails in interactive mode

  • Priority: P2
  • Location: experimental/postgres/cmd/render.go:70-72
  • Scenario: When interactive is true and row count exceeds the threshold, render.go:70-72 does return tableview.Run(...) directly. Any TUI startup or runtime error causes the function to return that error immediately; the static tabwriter path at lines 74-85 is never reached. The query completed successfully but the user sees no data. Reachable from query.go:206 whenever stdoutTTY && cmdio.IsPromptSupported(ctx).
  • Potential solution: Capture the error from tableview.Run; on non-nil error, fall through to the static tabwriter rendering path instead of returning the error to the caller.

stdoutTTY uses SupportsColor instead of a pure TTY check, causing JSON fallback when NO_COLOR is set

  • Priority: P2
  • Location: experimental/postgres/cmd/query.go:139
  • Scenario: A user on a real interactive TTY sets NO_COLOR=1 and does not pass --output explicitly. cmdio.SupportsColor returns isTTY(w) && c.color (libs/cmdio/capabilities.go:47-49); c.color is false when NO_COLOR is set (capabilities.go:29). This makes stdoutTTY false. experimental/libs/sqlcli/output.go:76-78 then auto-falls back from text to JSON, contradicting both the documented intent of stdoutTTY ("true if stdout is a terminal", output.go:57) and the command's help text.
  • Potential solution: Replace cmdio.SupportsColor(ctx, cmd.OutOrStdout()) with a pure terminal check (e.g., term.IsTerminal(int(os.Stdout.Fd())) or a new cmdio.IsOutputTTY helper) for the stdoutTTY variable; keep SupportsColor only for color-rendering decisions.

🔍 Reviewed by nitpicker

@simonfaltum
Merge branch 'simonfaltum/postgres-query-pr3-multi-input' into simonf…
6937e7b 
…altum/postgres-query-pr4-cancel
@simonfaltum
Address PR 4 nitpicker finding: TUI fallback on tableview.Run error
a275d48 
If tableview.Run errors (TUI startup failure, terminal resize race,
etc.), fall through to the static tabwriter path instead of returning
the error to the caller. Without the fallback, a successful query
surfaces as "viewer failed" with no data — the user paid for the
query but doesn't see the rows.

Co-authored-by: Isaac
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 1, 2026 07:03 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 1, 2026 07:03 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arsenyinfo arsenyinfo arsenyinfo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@simonfaltum @arsenyinfo