Do not swallow errors in simple query. Fixing transaction descriptors. Read envchanges correctly. #105
Conversation
This fixes a problem where we might have warnings coming before errors in multiple resultset queries, when executing with `simple_query`. Example: ``` CREATE UNIQUE INDEX [X] ON [table] ([column]); ``` In this case, if our column breaks unique validations, we'd expect `simple_query` to return an error. If the column is of type `varchar` and the length is above threshold, the first token we get back is a warning of index size. We used to stop fetching data at this point, and our query would be successful even though we actually had an error in the wire.
Only the `BeginTransaction` holds the descriptor. Commit, rollback et.al. do not hold any data, and we can just zero the descriptor if having these events. This has been causing weird bugs when running MARS with transactions somewhere in the middle.
In certain cases envchange token contains data we don't need. We still must read all of it to not cause trouble when reading the next token.
f3cb73a to
fcb18ef
Compare
| @@ -10,19 +10,19 @@ pub struct TokenRpcRequest<'a> { | |||
| proc_id: RpcProcIdValue<'a>, | |||
| flags: RpcOptionFlags, | |||
| params: Vec<RpcParam<'a>>, | |||
| transaction_id: u64, | |||
| transaction_desc: [u8; 8], | |||
There was a problem hiding this comment.
Is that change motivated by endianness concerns? I can see we write that with put_slice() instead of put_u64_le() now.
There was a problem hiding this comment.
I just do whatever the JDBC driver does. We always just write bytes to the message header, so there is no reason to convert that to an arbitrary number.
| // We read all the bytes now, due to whatever environment change tokens | ||
| // we read, they might contain padding zeroes in the end we must | ||
| // discard. | ||
| let mut bytes = vec![0; len]; |
There was a problem hiding this comment.
This is one place where read-buf will come in handy — free performance improvement when it's stable.
There was a problem hiding this comment.
Yeah, except we don't luckily read env tokens that often, and they're all quite short.
Still excited about that RFC.
| Some(ReceivedToken::DoneInProc(done)) | ||
| | Some(ReceivedToken::DoneProc(done)) | ||
| | Some(ReceivedToken::Done(done)) => { | ||
| if !done.has_more() { |
There was a problem hiding this comment.
So if I understand this correctly, this is the part about swallowed errors?
There was a problem hiding this comment.
Yeah. We must go through the whole stream always. We might get any of the done tokens, but they have a bitflag set if there's more data to be come.
| src.read_u8().await?; | ||
| let desc = src.read_u64_le().await?; | ||
| let len = buf.read_u8()?; | ||
| assert!(len == 8); |
There was a problem hiding this comment.
I don't know if this is really needed. The JDBC code is extremely careful asserting all the things. The TDS standard dictates how the descriptor is always 8 bytes. And the only implementation is the SQL Server from Microsoft, which I highly doubt would send 9 bytes descriptor every now and then...
Fixing three bugs.
Error swallowing
This fixes a problem where we might have warnings coming before errors in multiple resultset queries, when executing with
simple_query.Example:
CREATE UNIQUE INDEX [X] ON [table] ([column]);In this case, if our column breaks unique validations, we'd expect
simple_queryto return an error.If the column is of type
varcharand the length is above threshold, the first token we get back is a warning of index size. We used to stop fetching data at this point, and our query would be successful even though we actually had an error in the wire.Transaction descriptor reading
Only the
BeginTransactionholds the descriptor. Commit, rollback et.al. do not hold any data, and we can just zero the descriptor if having these events.This has been causing weird bugs when running MARS with transactions somewhere in the middle.
Environment change token reads
This was tricky. When we get
TokenEnvChange, it's typically in the following form:What we must do is to read ALL bytes first with the first length, then use this buffer to construct whatever token we need. There might be some bytes left in the buffer, which we just throw away in the end.