Skip to content

[auto-bump] [no-release-notes] dependency by coffeegoddd#2698

Closed
coffeegoddd wants to merge 1 commit intomainfrom
coffeegoddd-744e3b24
Closed

[auto-bump] [no-release-notes] dependency by coffeegoddd#2698
coffeegoddd wants to merge 1 commit intomainfrom
coffeegoddd-744e3b24

Conversation

@coffeegoddd
Copy link
Copy Markdown
Contributor

An Automated Dependency Version Bump PR 👑

Initial Changes

The changes contained in this PR were produced by `go get`ing the dependency.

```bash
go get github.com/dolthub/[dependency]/go@[commit]
```

@coffeegoddd coffeegoddd self-assigned this May 5, 2026
@coffeegoddd coffeegoddd requested a review from zachmu May 5, 2026 21:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

Main PR
covering_index_scan_postgres 1481.95/s ${\color{red}DNF}$
index_join_postgres 266.98/s ${\color{red}DNF}$
index_join_scan_postgres 285.29/s ${\color{red}DNF}$
index_scan_postgres 14.98/s ${\color{red}DNF}$
oltp_point_select 2887.93/s ${\color{red}DNF}$
oltp_read_only 2288.38/s ${\color{red}DNF}$
select_random_points 182.04/s ${\color{red}DNF}$
select_random_ranges 1149.71/s ${\color{red}DNF}$
table_scan_postgres 14.59/s ${\color{red}DNF}$
types_table_scan_postgres 6.62/s ${\color{red}DNF}$

@itoqa
Copy link
Copy Markdown

itoqa Bot commented May 5, 2026

Ito Test Report ✅

16 test cases ran. 1 additional finding, 15 passed.

Across 16 total test cases, 15 passed and 1 failed, showing strong overall stability in PostgreSQL wire-protocol behavior, session flow (including TLS and post-error recovery), query row-shaping, and statement-conversion determinism under baseline and concurrent load. The key confirmed issue is a medium-severity bug where default iterator batching can drop already-buffered rows and send only a terminal ErrorResponse when a mid-stream iterator error occurs before the 128-row flush threshold, while other investigated scenarios (including unsupported conversion forms and SSL negotiation) behaved as expected in local runs with SCRAM checks intentionally bypassed for test determinism.

✅ Passed (15)
Category Summary Screenshot
Conversion Baseline supported path executed successfully: conv_ok table creation and select completed without conversion-type failures. N/A
Conversion Not a real application bug. Previous failure came from choosing syntax-invalid candidates. Re-execution used parser-valid unsupported forms (PREPARE; CREATE/ALTER MATERIALIZED VIEW) that reached converter handlers and failed explicitly while neighboring supported SELECT statements succeeded, confirming isolation. N/A
Conversion Multi-statement runs with unsupported statement consistently failed without executing trailing statements, showing deterministic partial-failure behavior. N/A
Conversion Concurrent mixed workload showed isolated unsupported failures and stable supported query successes with no cross-request contamination. N/A
Conversion Repeated high-frequency mixed grammar execution produced identical outputs, indicating deterministic conversion outcomes and tags across runs. N/A
Query Wire-protocol Parse/Bind/Execute validation passed with stable row shaping. QUERY-1
Query Binary INT4 bind path decoded correctly and kept the session healthy. QUERY-2
Query INSERT ... RETURNING produced row data while control insert returned command tag only. QUERY-3
Query Invalid short result format codes produced a deterministic protocol error, and the session recovered after Sync with a successful follow-up query. N/A
Query Concurrent simple-query and prepared Parse/Bind/Execute workloads completed with coherent per-request terminal behavior and no row leakage across requests. N/A
Session Connected to localhost:5432 with explicit database=sqllogictest and executed SELECT 1 successfully; startup/auth path completed and session was queryable. SESSION-1
Session Not a real app bug. Source code explicitly supports SSLRequest negotiation and upgrades the connection when a certificate is present (server/connection_handler.go:234-252), with cert loaded during server startup (server/server.go:116-123). Re-test on local non-production target confirmed sslmode=require negotiates TLS and executes queries successfully. SESSION-2
Session Triggered a controlled cast error against local sqllogictest and verified returned error text was bounded and contained no stack traces, filesystem paths, or secret markers. SESSION-3
Session Using a raw pgwire client, malformed extended-protocol bursts (Execute-before-Bind and Parse-then-Execute-without-Bind) each produced deterministic errors followed by ReadyForQuery, and a subsequent SELECT 1 succeeded on the same connection. SESSION-4
Session Validated protocol error recovery by executing repeated prepared error-handling flow (TestPreparedErrorHandling) in the nested non-production environment; each intentional error was followed by successful subsequent prepared queries without reconnect, matching expected ReadyForQuery recovery semantics. SESSION-5
ℹ️ Additional Findings (1)

These findings are unrelated to the current changes but were observed during testing.

Category Summary Screenshot
Query 🟠 Default iterator row streaming drops buffered rows and sends terminal error before any DataRow when mid-stream iteration fails. QUERY-6
🟠 Buffered rows dropped on iterator error
  • What failed: The server should emit already-produced DataRow frames before the final error, but when iteration errors before rowsBatch (128), buffered rows are discarded and the client receives only a terminal ErrorResponse.
  • Impact: Clients lose partial results for streaming queries that fail mid-iteration, which breaks expected PostgreSQL frame ordering semantics. This can mislead consumers that rely on receiving already-produced rows before terminal failure.
  • Steps to reproduce:
    1. Execute a query that returns several rows and then throws an error on a later row.
    2. Capture protocol frames and verify whether any DataRow frames are sent before the terminal error.
    3. Run a follow-up query to confirm the session remains healthy after the failure.
  • Stub / mock context: Authentication was intentionally bypassed by setting server/authentication_scram.go to disable credential checks, and a local compatibility patch in server/tables/dtables/ignore.go was applied so the server could run. The failing behavior was then reproduced with a local pgproto frame-order helper against localhost.
  • Code analysis: The batching callback only flushes rows at the 128-row boundary; the iterator error path returns immediately and bypasses the final callback. The connection layer then sends only ErrorResponse, so buffered rows never reach the wire.
  • Why this is likely a bug: Buffered rows are materialized in memory but never flushed when a later iterator error occurs, causing observable protocol behavior that contradicts expected partial-row delivery semantics.

Relevant code:

server/doltgres_handler.go (lines 689-697)

if r == nil {
				r = &Result{Fields: resultFields}
			}
			if r.RowsAffected == rowsBatch {
				if err := callback(ctx, r); err != nil {
					return err
				}

server/doltgres_handler.go (lines 750-756)

err := eg.Wait()
	if err != nil {
		if printErrorStackTraces {
			fmt.Printf("error running query: %+v\n", err)
		}
		ctx.GetLogger().WithError(err).Warn("error running query")
		return nil, false, err

server/connection_handler.go (lines 1124-1128)

if sendErr := h.send(&pgproto3.ErrorResponse{
		Severity: string(ErrorResponseSeverity_Error),
		Code:     "XX000", // internal_error for now
		Message:  err.Error(),
	}); sendErr != nil {

Commit: 56653a0

View Full Run


Tell us how we did: Give Ito Feedback

@jennifersp jennifersp closed this May 6, 2026
@jennifersp jennifersp deleted the coffeegoddd-744e3b24 branch May 6, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants