Support compound COPY FROM STDIN queries - #3321
Conversation
|
SummaryCoverage spans normal bulk loading and multi-statement flows, transaction visibility and rollback, statement ordering, connection recovery, replay consistency, and adversarial malformed or interrupted input. Overall behavior is healthy across both expected operations and failure-recovery paths, with one edge case involving empty input identified outside this change. Safe to merge — the only observed issue is a medium-severity empty-input edge case that is not attributable to this PR, with no regressions or newly introduced failures linked to the change. It is a flag for later rather than a merge blocker. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Empty COPY blocks the next statement
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
@fulghum DOLT
|
1b3fbc9 to
b6bacde
Compare
|
Commit: SummaryCoverage spans successful data loading and commit behavior, rollback and recovery after failures, cross-connection visibility, stream separation, and adversarial timing or overlapping-protocol cases. The tested behavior is broadly healthy across transaction and protocol edge cases, with one gap in handling concurrent commands during an unfinished upload. Merge with caution — the PR has a medium-severity, attributable protocol-state defect that can allow commands to execute out of order during an unfinished data upload, potentially causing incorrect results or partial changes. The remaining passing coverage supports the main commit, rollback, recovery, and stream-isolation paths, but this concurrency issue is not a merge blocker at the assigned severity. Tests run by ItoTip Reply with @itoqa to send us feedback on this test run. |
| @@ -462,6 +470,9 @@ func (h *ConnectionHandler) handleMessage(msg pgproto3.Message) (stop, endOfMess | |||
| // We don't buffer output, so Flush is a no-op | |||
| return false, false, nil | |||
| case *pgproto3.Query: | |||
There was a problem hiding this comment.
Extended messages run during pending COPY
What failed: The server does not reject extended-protocol messages while a COPY upload is waiting for data. It runs those messages before the original query is complete.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Database clients that upload data through COPY can have other commands run before the upload finishes. This may change transaction state or run work out of order, causing incorrect results or partial changes.
- Steps to Reproduce:
- Open a PostgreSQL wire connection and send a compound simple Query containing COPY FROM STDIN so the server returns CopyInResponse.
- Before sending CopyDone, send an extended-protocol Parse, Bind, or Execute message, or send Sync.
- Observe that the server dispatches the message instead of rejecting it as work received during the pending COPY.
- Complete or inspect the COPY exchange and the connection responses; the extended operation has already changed protocol or transaction state while the simple Query was suspended.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR introduces ConnectionHandler.activeSimpleQuery and stores the multi-statement query in it at server/connection_handler.go:552-553. When handleQueryOutsideEngine reaches an injected COPY FROM STDIN statement, server/connection_handler.go:694-700 returns endOfMessages=false, so resumeSimpleQuery returns at lines 586-596 with activeSimpleQuery still populated and the connection remains in the COPY wait. The new dispatcher check at server/connection_handler.go:472-475 rejects only a second pgproto3.Query. The following cases at lines 478-485 dispatch Parse, Bind, and Execute directly, while Sync at lines 464-468 clears waitForSync and calls commitImplicitTransaction without checking activeSimpleQuery. Those handlers perform real work: handleParse prepares and stores statements at lines 713-785, handleBind creates portal state at lines 817-872, and handleExecute starts an implicit transaction and runs the portal at lines 875-917. Therefore the new suspended-query state is not exclusive to COPY messages: extended work can execute in the middle of the compound query and Sync can alter transaction state before CopyDone resumes the saved statement list. The smallest practical fix is to add one pending-COPY guard in the dispatcher for Parse, Describe, Bind, Execute, Sync, and other non-COPY messages, preserving only the COPY data, done, fail, and termination messages needed to finish or abort the suspended operation.
- Why this is likely a bug: The test's intended protocol rule is clear: while COPY FROM STDIN is pending, the connection must wait for CopyData, CopyDone, or CopyFail rather than process another operation. The existing Query guard demonstrates that the new activeSimpleQuery state is meant to block overlapping work, but its placement leaves the extended-protocol cases unguarded. The source path is sufficient to confirm the defect even though the available TestWireTypesSending run has no pending-COPY scenario and the later browser retry was unavailable. A targeted dispatcher guard is sufficient; no broad transaction rewrite is required.
Relevant code
server/connection_handler.go:472-485
case *pgproto3.Query:
if h.activeSimpleQuery != nil {
return false, true, errors.New("query received while a COPY FROM STDIN operation is in progress")
}
...
case *pgproto3.Parse:
return false, false, h.handleParse(message)
...
case *pgproto3.Execute:
return false, false, h.handleExecute(message)server/connection_handler.go:552-565
h.activeSimpleQuery = &simpleQueryExecution{statements: queries}
return h.resumeSimpleQuery()
...
if endOfMessages || err != nil {
h.activeSimpleQuery = nil
}server/connection_handler.go:694-700
if injectedStmt.Stdin {
return true, false, h.handleCopyFromStdinQuery(injectedStmt, h.Conn())
}server/connection_handler.go:464-468
case *pgproto3.Sync:
h.waitForSync = false
return false, true, h.commitImplicitTransaction()server/connection_handler.go:713-785
func (h *ConnectionHandler) handleParse(message *pgproto3.Parse) error { ... h.preparedStatements[message.Name] = PreparedStatementData{ ... } }server/connection_handler.go:817-917
func (h *ConnectionHandler) handleBind(...) ... { ... h.portals[...] = PortalData{...} ... }
func (h *ConnectionHandler) handleExecute(...) ... {
if err := h.startImplicitTransaction(query); err != nil { ... }
err = h.doltgresHandler.ComExecuteBound(...)
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Extended messages run during pending COPY**
**What failed:** The server does not reject extended-protocol messages while a COPY upload is waiting for data. It runs those messages before the original query is complete.
- **Impact:** Database clients that upload data through COPY can have other commands run before the upload finishes. This may change transaction state or run work out of order, causing incorrect results or partial changes.
- **Steps to reproduce:**
1. Open a PostgreSQL wire connection and send a compound simple Query containing COPY FROM STDIN so the server returns CopyInResponse.
2. Before sending CopyDone, send an extended-protocol Parse, Bind, or Execute message, or send Sync.
3. Observe that the server dispatches the message instead of rejecting it as work received during the pending COPY.
4. Complete or inspect the COPY exchange and the connection responses; the extended operation has already changed protocol or transaction state while the simple Query was suspended.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR introduces ConnectionHandler.activeSimpleQuery and stores the multi-statement query in it at server/connection_handler.go:552-553. When handleQueryOutsideEngine reaches an injected COPY FROM STDIN statement, server/connection_handler.go:694-700 returns endOfMessages=false, so resumeSimpleQuery returns at lines 586-596 with activeSimpleQuery still populated and the connection remains in the COPY wait. The new dispatcher check at server/connection_handler.go:472-475 rejects only a second pgproto3.Query. The following cases at lines 478-485 dispatch Parse, Bind, and Execute directly, while Sync at lines 464-468 clears waitForSync and calls commitImplicitTransaction without checking activeSimpleQuery. Those handlers perform real work: handleParse prepares and stores statements at lines 713-785, handleBind creates portal state at lines 817-872, and handleExecute starts an implicit transaction and runs the portal at lines 875-917. Therefore the new suspended-query state is not exclusive to COPY messages: extended work can execute in the middle of the compound query and Sync can alter transaction state before CopyDone resumes the saved statement list. The smallest practical fix is to add one pending-COPY guard in the dispatcher for Parse, Describe, Bind, Execute, Sync, and other non-COPY messages, preserving only the COPY data, done, fail, and termination messages needed to finish or abort the suspended operation.
- **Why this is likely a bug:** The test's intended protocol rule is clear: while COPY FROM STDIN is pending, the connection must wait for CopyData, CopyDone, or CopyFail rather than process another operation. The existing Query guard demonstrates that the new activeSimpleQuery state is meant to block overlapping work, but its placement leaves the extended-protocol cases unguarded. The source path is sufficient to confirm the defect even though the available TestWireTypesSending run has no pending-COPY scenario and the later browser retry was unavailable. A targeted dispatcher guard is sufficient; no broad transaction rewrite is required.
**Relevant code:**
`server/connection_handler.go:472-485`
~~~go
case *pgproto3.Query:
if h.activeSimpleQuery != nil {
return false, true, errors.New("query received while a COPY FROM STDIN operation is in progress")
}
...
case *pgproto3.Parse:
return false, false, h.handleParse(message)
...
case *pgproto3.Execute:
return false, false, h.handleExecute(message)
~~~
`server/connection_handler.go:552-565`
~~~go
h.activeSimpleQuery = &simpleQueryExecution{statements: queries}
return h.resumeSimpleQuery()
...
if endOfMessages || err != nil {
h.activeSimpleQuery = nil
}
~~~
`server/connection_handler.go:694-700`
~~~go
if injectedStmt.Stdin {
return true, false, h.handleCopyFromStdinQuery(injectedStmt, h.Conn())
}
~~~
`server/connection_handler.go:464-468`
~~~go
case *pgproto3.Sync:
h.waitForSync = false
return false, true, h.commitImplicitTransaction()
~~~
`server/connection_handler.go:713-785`
~~~go
func (h *ConnectionHandler) handleParse(message *pgproto3.Parse) error { ... h.preparedStatements[message.Name] = PreparedStatementData{ ... } }
~~~
`server/connection_handler.go:817-917`
~~~go
func (h *ConnectionHandler) handleBind(...) ... { ... h.portals[...] = PortalData{...} ... }
func (h *ConnectionHandler) handleExecute(...) ... {
if err := h.startImplicitTransaction(query); err != nil { ... }
err = h.doltgresHandler.ComExecuteBound(...)
}
~~~
zachmu
left a comment
There was a problem hiding this comment.
Good fix, but having this many parallel state tracking fields is hard to keep track of. I think I would like to see an attempt to unify the various bits of state tracking into a single type which makes it clear how the transition between states in the machine can take place. It's all spread out through the connection handler code right now and very confusing to follow, and this PR makes it even more confusing.
| // We don't buffer output, so Flush is a no-op | ||
| return false, false, nil | ||
| case *pgproto3.Query: | ||
| if h.activeSimpleQuery != nil { |
There was a problem hiding this comment.
This condition and the following error message are a bit counterintuitive, could use a comment. Also see the Ito note.
| // COPY DATA messages from the client to import data into tables. | ||
| copyFromStdinState *copyFromStdinState | ||
| // activeSimpleQuery is the current multi-statement simple query execution. | ||
| activeSimpleQuery *simpleQueryExecution |
There was a problem hiding this comment.
Should add a comment about this being mutually exclusive with the other field. Or maybe combine these into a new sessionState struct that includes transactionState?


Compound simple-query messages containing
COPY FROM STDINstopped executing after the COPY operation completed, so later statements were skipped and the server sentReadyForQuerytoo early. COPY data was also committed independently instead of remaining within the compound query’s implicit or explicit transaction.This change retains the active simple-query execution while COPY waits for client data, resumes the remaining statements after
CopyDone, and preserves the enclosing transaction’s commit and rollback behavior.CopyFailnow aborts the suspended query, rolls back implicit work, and marks explicit transactions failed with SQLSTATE57014.The regression replayer also incorrectly combined all recorded COPY data into one stream and resent it for every
CopyInResponse. It now preserves COPY stream boundaries and sends each recorded input exactly once, in statement order.