simulate server side cursors #228
Conversation
go/mysql/conn.go
Outdated
| @@ -1030,6 +1050,7 @@ func (c *Conn) handleNextCommand(handler Handler) error { | |||
| } | |||
| } | |||
| case ComPrepare: | |||
| log.Info("Received COM_PREPARE") | |||
There was a problem hiding this comment.
The server connection has become quite a bit more stateful now, because it can be in this cursor-exists state with the query running in the background. I think almost all of these cases need to check for conn.cs != nil and do the right thing...I'm not exactly sure what the right thing is for each of them. Maybe connection out of sync error and close the connection?
Seems like ComStmtReset should definitely cancel the query, ensure the query is canceled by selecting on its done channel, and only return success after that's done.
conn.StatusFlags will need to be handled carefully across anywhere the conn.cs can go from != nil to == nil, since we need to make sure CursorExists is getting cleared.
go/mysql/conn.go
Outdated
| return err | ||
| } | ||
| // indicate that there is no more data, and prevent blocking from reading c.cs.done for errors again | ||
| c.cs.noData = true |
|
|
||
| next chan *sqltypes.Result | ||
| done chan error | ||
| quit chan error |
There was a problem hiding this comment.
As written we don't currently cancel the query anywhere by sending on this channel. As mentioned below though, I think we actually need to sometimes. When we do do that, we probably need to select like:
select {
conn.cs.quit <- errToCancelTheQueryHere:
errWhichWonTheRace := <- conn.cs.done:
}
in order to avoid deadlocking...
go/mysql/conn.go
Outdated
| if !ok { | ||
| // check if there was an error | ||
| if err = <- c.cs.done; err != nil { | ||
| if werr := c.writeErrorPacketFromError(err); werr != nil { |
| case ComStmtExecute: | ||
| // flush is called at the end of this block. | ||
| // To simplify error handling, we do not | ||
| // encapsulate it with a defer'd func() | ||
| c.startWriterBuffering() | ||
|
|
||
| queryStart := time.Now() | ||
| stmtID, _, err := c.parseComStmtExecute(c.PrepareData, data) | ||
| stmtID, cursorType, err := c.parseComStmtExecute(c.PrepareData, data) |
There was a problem hiding this comment.
I think we need to prevent ComStmtExecute, ComStmtPrepare and ComStmtQuery while a single cursor is outstanding. Thoughts?
There was a problem hiding this comment.
I think that makes sense for ComStmtExecute, since that would open another cursor, potentially causing problems, especially since we're only going to support one cursor at a time. I don't think ComStmtPrepare would be affected by any open cursors. ComStmtQuery might cause problems if the query affects the table the cursor is reading from, like inserting or updating rows.
There was a problem hiding this comment.
Yeah, I think there are situations where it would be fine to let Prepares run, but my concerns is with adding concurrency on the session when there wasn't any previously. For example, I'm pretty sure the database catalog has a transactional view of the schemas of the tables being queried, and that's referenced in the analyzer. It seems most expedient to just disallow it for now, assuming existing clients don't rely on this.
go/mysql/conn.go
Outdated
|
|
||
| go func() { | ||
| err = handler.ComStmtExecute(c, prepare, func(qr *sqltypes.Result) error { | ||
| defer func(){ |
There was a problem hiding this comment.
This whole defer block goes outside the ComStmtExecute call.
There was a problem hiding this comment.
What's our testing strategy here in general? I don't think we have enginetests which currently test this code path. We need to make sure we have tests in place that are testing this code path with larger-than-128-rows result sets, where the placement of this defer would have been clearly wrong.
go/mysql/conn.go
Outdated
| if !ok { | ||
| // check if there was an error | ||
| if err, ok = <- c.cs.done; ok { | ||
| close(c.cs.done) |
There was a problem hiding this comment.
If we ever want this to close, we should have the sender close it. This server code should be structured to only ever successfully receive from it once though.
| case ComStmtExecute: | ||
| // flush is called at the end of this block. | ||
| // To simplify error handling, we do not | ||
| // encapsulate it with a defer'd func() | ||
| c.startWriterBuffering() | ||
|
|
||
| queryStart := time.Now() | ||
| stmtID, _, err := c.parseComStmtExecute(c.PrepareData, data) | ||
| stmtID, cursorType, err := c.parseComStmtExecute(c.PrepareData, data) |
There was a problem hiding this comment.
Yeah, I think there are situations where it would be fine to let Prepares run, but my concerns is with adding concurrency on the session when there wasn't any previously. For example, I'm pretty sure the database catalog has a transactional view of the schemas of the tables being queried, and that's referenced in the analyzer. It seems most expedient to just disallow it for now, assuming existing clients don't rely on this.
To be able to use the newest MySQL foreign data wrapper for Postgres, we have to support server side cursors.
This PR emulates them, and hopefully gets us far enough...
Additionally, this includes a fix for Windows clients (and other clients that don't have the DeprecatedEOF client capability flag set) where we send an extra EOF packet when we shouldn't.
Fix for: dolthub/dolt#5441
And this: dolthub/dolt#3029
This one too (kinda): dolthub/dolt#4840