kvcoord: remove an artifact of unsetting WriteTooOld flag #85101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It seems like during 20.1 release cycle we had a change so that the errors don't carry `WriteTooOld` flag set to `true` and now only the BatchResponse can have that. We kept the ability of unsetting that flag so that in a mixed-version scenario we wouldn't run into any issues, but now that unsetting behavior is no longer needed and is removed in this commit. My particular interest in removing this is because it is the only modification of the `SetupFlowRequest` proto (which carries the `Txn` proto inside) that occurs during the distributed query setup, and I want to have more parallelization there. Release note: None
|
This change is |
andreimatei
reviewed
Jul 27, 2022
complete! 1 of 0 LGTMs obtained
|
TFTR! bors r+ |
|
Build succeeded: |
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Sep 9, 2022
Touches cockroachdb#85711, fixing one of the failure modes. In cockroachdb#85101 we deleted code in the span refresher interceptor that WriteTooOld flags. We did so assuming these flags were only set in 19.2 servers, but that's not the case. Since we were no longer terminating the flag, in rare occasions we hit an assertion that this flag was not set on batch requests. Removing this termination code made this possible, see 85711#issuecomment-1240064777 for more details. Release note: None
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Sep 20, 2022
Touches cockroachdb#85711 fixing one of the failure modes. In cockroachdb#85101 we deleted code in the span refresher interceptor that terminated WriteTooOld flags. We did so assuming these flags were only set in 19.2 servers, but that's not the case -- TestWTOBitTerminatedOnErrorResponses demonstrates that it's possible for the server to return error responses with the bit set if a response is combined with an error from another request in the same batch request. Since we were no longer terminating the flag, it was possible to update the TxnCoordSender's embedded txn with this bit, an then use it when issuing subsequent batch requests -- something we were asserting against. Release note: None Release justification: Bug fix
craig bot
pushed a commit
that referenced
this pull request
Sep 20, 2022
87739: kvcoord: (partially) de-flake tpcc/multiregion r=irfansharif a=irfansharif Touches #85711 fixing one of the failure modes. In #85101 we deleted code in the span refresher interceptor that terminated WriteTooOld flags. We did so assuming these flags were only set in 19.2 servers, but that's not the case -- TestWTOBitTerminatedOnErrorResponses demonstrates that it's possible for the server to return error responses with the bit set if a response is combined with an error from another request in the same batch request. Since we were no longer terminating the flag, it was possible to update the TxnCoordSender's embedded txn with this bit, an then use it when issuing subsequent batch requests -- something we were asserting against. Release note: None Release justification: Bug fix 88174: rowenc: fix needed column families computation for secondary indexes r=yuzefovich a=yuzefovich Previously, when determining the "minimal set of column families" required to retrieve all of the needed columns for the scan operation we could incorrectly not include the special zeroth family into the set. The KV for the zeroth column family is always present, so it might need to be fetched even when it's not explicitly needed when the "needed" column families are all nullable. Before this patch the code for determining whether all of the needed column families are nullable incorrectly assumed that all columns in a family are stored, but this is only true for the primary indexes - for the secondary indexes only those columns mentioned in `STORING` clause are actually stored (apart from the indexed and PK columns). As a result we could incorrectly not fetch a row if: - the unique secondary index is used - the needed column has a NULL value - all non-nullable columns from the same column family as the needed column are not stored in the index - other column families are not fetched. This is now fixed by considering only the set of stored columns. The bug seems relatively minor since it requires a multitude of conditions to be met, so I don't think it's a TA worthy. Fixes: #88110. Release note (bug fix): Previously, CockroachDB could incorrectly not fetch rows with NULL values when reading from the unique secondary index when multiple column families are defined for the table and the index doesn't store some of the NOT NULL columns. 88182: sql: fix relocate commands with NULLs r=yuzefovich a=yuzefovich Previously, we would crash when evaluating `EXPERIMENTAL_RELOCATE` commands when some of the values involved where NULL, and this is now fixed. There is no release note since the commands are "experimental" after all. Fixes: #87371. Release note: None 88187: util/growstack: increase stack growth for 1.19 r=nvanbenschoten a=ajwerner Now that we've adopted go 1.19, we notice that the performance is much worse (~8%) than we observed in go 1.18. Interestingly, we observe in profiles that we spend a lot more time increasing our stack size underneath request evaluation. This implied to me that some part of this is probably due to the runtime's new stack growth behavior. Perhaps what is going on is that the initial stacks are now smaller than they used to be so when we grow it, we grow it by less than we need to. I ran a benchmark that seems to indicate that this theory is true. I'd like to merge this to master and then backport it after we collect some more data. We never released this, so no note. Touches #88038 Release note: None 88195: colbuilder: don't use optimized IN operator for empty tuple r=yuzefovich a=yuzefovich This commit makes it so that we don't use the optimized IN operator for empty tuples since they handle NULLs incorrectly. This wasn't supposed to happen already due to 9b590d3 but there we only looked at the type and not at the actual datum. This is not a production bug since the optimizer normalizes such expressions away. Fixes: #88141. Release note: None Co-authored-by: irfan sharif <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Andrew Werner <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this pull request
Sep 20, 2022
Touches #85711 fixing one of the failure modes. In #85101 we deleted code in the span refresher interceptor that terminated WriteTooOld flags. We did so assuming these flags were only set in 19.2 servers, but that's not the case -- TestWTOBitTerminatedOnErrorResponses demonstrates that it's possible for the server to return error responses with the bit set if a response is combined with an error from another request in the same batch request. Since we were no longer terminating the flag, it was possible to update the TxnCoordSender's embedded txn with this bit, an then use it when issuing subsequent batch requests -- something we were asserting against. Release note: None Release justification: Bug fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems like during 20.1 release cycle we had a change so that the
errors don't carry
WriteTooOldflag set totrueand now only theBatchResponse can have that. We kept the ability of unsetting that flag
so that in a mixed-version scenario we wouldn't run into any issues, but
now that unsetting behavior is no longer needed and is removed in this
commit.
My particular interest in removing this is because it is the only
modification of the
SetupFlowRequestproto (which carries theTxnproto inside) that occurs during the distributed query setup, and I want
to have more parallelization there.
Release note: None