Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kvclient: disallow savepoint rollback after most errors #47724

Merged
merged 4 commits into from
Apr 23, 2020
Merged

kvclient: disallow savepoint rollback after most errors #47724

merged 4 commits into from
Apr 23, 2020

Commits on Apr 23, 2020

  1. kvclient: remove duplicated assertion 

    An assertion was duplicating one a few lines above.

Release note: None
    @andreimatei
    andreimatei committed Apr 23, 2020
    Configuration menu
    Copy the full SHA
    6ef2871 View commit details
    Browse the repository at this point in the history

  2. sql: improve a test 

    The test was trying to inject a retriable error, but it wasn't doing it
properly. It was injecting a TransactionRetryWithProtoRefreshError, but
that error is supposed to be generated at a higher layer. It was
inconsequential, but it's about to become consequential in the next
commit when rolling back to a savepoint is no longer allowed after
random errors. At that point, the TxnCoordSender will not recognize
TransactionRetryWithProtoRefreshError.

Release note: None
    @andreimatei
    andreimatei committed Apr 23, 2020
    Configuration menu
    Copy the full SHA
    28196b6 View commit details
    Browse the repository at this point in the history

  3. kvclient: further clarify DistSender ambiguity comments 

    Adding more words abouts the morass, after the previous rant in cockroachdb#47580.

Release note: None
    @andreimatei
    andreimatei committed Apr 23, 2020
    Configuration menu
    Copy the full SHA
    0db4405 View commit details
    Browse the repository at this point in the history

  4. kvclient: disallow savepoint rollback after most errors 

    This patch changes the handling of ConditionFailedErrors (after failed
CPuts). It used to be the case that a txn could not continue sending
requests after such an error (like after any other error). It is now
allowed for a transaction to continue sending requests after this
particular error because, in particular, we want to allow rollback to
savepoints after it (see below). More broadly, there's no particular
reason to not allow the client to continue, as long as it knows what
it's doing. In fact, it's long been debatable whether
ConditionFailedErrors should be errors at all, rather than another type
of result.
One caveat is that clients chosing to swallow this error need to be
cautious of mixed success conditions, if that's applicable to the batch
they have sent.
Note that at the SQL level, a CPut error will still cause connection to
be in an error state until a ROLLBACK.

This patch also disallows rolling back to savepoints after a request
returned any other error but ConditionFailedError. It used to be the
case that rolling back to a savepoint was allowed after most errors, on
the argument that our rollback mechanism (the ignored seq nums) is
powerful enough to undo any effects of requests evaluated since the
savepoint. Unfortunately, it wasn't safe. The way in which we overwrite
intents and update their timestamp in the process, coupled with the
transaction coordinator not being aware that an intent's timestamp was
updated after different ambiguous errors, can lead to partial commits.
See cockroachdb#47587.  We could more narrowly disallow rollbacks after ambiguous
errors, but the ambiguous cases are not trivial to isolate. I also want
to backport something, so this seems safer.

Fixes cockroachdb#47587

Release note (sql change): ROLLBACK TO SAVEPOINT is no longer permitted
after misc internal errors.
    @andreimatei
    andreimatei committed Apr 23, 2020
    Configuration menu
    Copy the full SHA
    65e8045 View commit details
    Browse the repository at this point in the history