Skip to content

rangefeed: improve assertion when txn refcount becomes negative#35772

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/betterAssert
Mar 15, 2019
Merged

rangefeed: improve assertion when txn refcount becomes negative#35772
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/betterAssert

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Mar 14, 2019

Informs #34600.

It is critical that unresolvedIntentQueue properly handle negative
reference counts to avoid leaking references before a rangefeed has
finished its resolved timestamp initialization scan. However, once this
scan is complete, reference counts on transactions should never drop
below zero. Such an occurrence would indicate that an intent was lost
either during the initial scan or somewhere in the logical ops stream.

Based on the stacktraces in #34600, I believe this is what is happening
because we can see that an MVCCCommitIntentOp is triggering the
assertion. This commit will make this more explicit and should hopefully
also fire more because it won't rely on the intent with a negative
refcount being the oldest intent tracked to fire.

Release note: None

Informs cockroachdb#34600.

It is critical that `unresolvedIntentQueue` properly handle negative
reference counts to avoid leaking references before a rangefeed has
finished its resolved timestamp initialization scan. However, once this
scan is complete, reference counts on transactions should never drop
below zero. Such an occurrence would indicate that an intent was lost
either during the initial scan or somewhere in the logical ops stream.

Based on the stacktraces in cockroachdb#34600, I believe this is what is happening
because we can see that an `MVCCCommitIntentOp` is triggering the
assertion. This commit will make this more explicit and should hopefully
also fire more because it won't rely on the intent with a negative
refcount being the oldest intent tracked to fire.

Release note: None
@nvb nvb requested review from a team and danhhz March 14, 2019 23:40
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@danhhz danhhz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Mar 15, 2019

I'm able to hit this assertion immediately! That means I probably shouldn't merge this until the root issue is fixed.

nvb added a commit to nvb/cockroach that referenced this pull request Mar 15, 2019
Fixes cockroachdb#34600.

Before this commit, rangefeed made the assumption that an aborted intent
implied an aborted transaction (after the rangefeed was in a steady-state).
It used this assumption to eagerly discard the bookkeeping for a transaction
after its first MVCCAbortIntent operation was seen as an optimization.

This was incorrect. A transaction can "abort" an intent even if it commits.
This is possible if the intent was only written at a previous epoch and not
written in the epoch that ended up finalizing the transaction. This commit
removes this assumption, which I confirmed fixes the resolved timestamp
regression using the new assertion from cockroachdb#35772.

I believe this could have two effects:
- it could trigger the assertion failure from cockroachdb#34600 if the transaction
  experiencing the incorrect assumption was the oldest being tracked at
  the time of the assumption.
- it could cause resolved timestamp stalls and prevent all future forward
  progress of resolved timestamps if the transaction experiencing the incorrect
  assumption was not the oldest being tracked at the time of the assumption.

Release note: None
@danhhz
Copy link
Copy Markdown
Contributor

danhhz commented Mar 15, 2019

Are you still thinking of merging this with the fix?

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Mar 15, 2019

Yes!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 15, 2019

Merge conflict (retrying...)

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 15, 2019

Build failed

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Mar 15, 2019

Flake due to #34081.

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 15, 2019

Merge conflict (retrying...)

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 15, 2019

Build failed

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Mar 15, 2019

Flaked due to #35484.

bors r+

craig bot pushed a commit that referenced this pull request Mar 15, 2019
35772: rangefeed: improve assertion when txn refcount becomes negative r=nvanbenschoten a=nvanbenschoten

Informs #34600.

It is critical that `unresolvedIntentQueue` properly handle negative
reference counts to avoid leaking references before a rangefeed has
finished its resolved timestamp initialization scan. However, once this
scan is complete, reference counts on transactions should never drop
below zero. Such an occurrence would indicate that an intent was lost
either during the initial scan or somewhere in the logical ops stream.

Based on the stacktraces in #34600, I believe this is what is happening
because we can see that an `MVCCCommitIntentOp` is triggering the
assertion. This commit will make this more explicit and should hopefully
also fire more because it won't rely on the intent with a negative
refcount being the oldest intent tracked to fire.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 15, 2019

Build succeeded

@craig craig bot merged commit 8e163ee into cockroachdb:master Mar 15, 2019
nvb added a commit to nvb/cockroach that referenced this pull request Mar 15, 2019
Fixes cockroachdb#34600.

Before this commit, rangefeed made the assumption that an aborted intent
implied an aborted transaction (after the rangefeed was in a steady-state).
It used this assumption to eagerly discard the bookkeeping for a transaction
after its first MVCCAbortIntent operation was seen as an optimization.

This was incorrect. A transaction can "abort" an intent even if it commits.
This is possible if the intent was only written at a previous epoch and not
written in the epoch that ended up finalizing the transaction. This commit
removes this assumption, which I confirmed fixes the resolved timestamp
regression using the new assertion from cockroachdb#35772.

I believe this could have two effects:
- it could trigger the assertion failure from cockroachdb#34600 if the transaction
  experiencing the incorrect assumption was the oldest being tracked at
  the time of the assumption.
- it could cause resolved timestamp stalls and prevent all future forward
  progress of resolved timestamps if the transaction experiencing the incorrect
  assumption was not the oldest being tracked at the time of the assumption.

Release note: None
craig bot pushed a commit that referenced this pull request Mar 15, 2019
35777: rangefeed: don't discard ref count for txn after intent abort r=nvanbenschoten a=nvanbenschoten

Fixes #34600.

Before this commit, rangefeed made the assumption that an aborted intent
implied an aborted transaction (after the rangefeed was in a steady-state).
It used this assumption to eagerly discard the bookkeeping for a transaction
after its first MVCCAbortIntent operation was seen as an optimization.

This was incorrect. A transaction can "abort" an intent even if it commits.
This is possible if the intent was only written at a previous epoch and not
written in the epoch that ended up finalizing the transaction. This commit
removes this assumption, which I confirmed fixes the resolved timestamp
regression using the new assertion from #35772.

I believe this could have two effects:
- it could trigger the assertion failure from #34600 if the transaction
  experiencing the incorrect assumption was the oldest being tracked at
  the time of the assumption.
- it could cause resolved timestamp stalls and prevent all future forward
  progress of resolved timestamps if the transaction experiencing the incorrect
  assumption was not the oldest being tracked at the time of the assumption.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
nvb added a commit to nvb/cockroach that referenced this pull request Mar 18, 2019
Fixes cockroachdb#35816.

The fix in cockroachdb#35777 inadvertently broke `txnPushAttempt` and its interaction with
transactions that were found to be aborted. This commit restores this handling
by reverting part of the assertion added in cockroachdb#35772 and splitting up the handling
of "aborted intents" from the handling of "aborted transactions".

Aborted transactions were always one of the most tricky situations to handle in
rangefeed. The challenge is that rangefeed may be tracking a transaction that
has been abandoned and may want to push it to unblock its resolved timestamp,
but it doesn't have a way to know where the aborted transaction's intents live
on its range. The aborted transaction record can't tell us this either. So the
only thing rangefeed can do is push the transaction and convince itself that the
aborted intents on its range can be safely ignored. This poses a problem for
accurate reference counting of intents though. We need to be able to discard a
transaction and still handle the eventual resolution of its aborted intents.

To handle this properly, the change splits up `MVCCAbortIntentOp` into
`MVCCAbortIntentOp` and `MVCCAbortTxnOp`. Before this change, the single
operation type was attempting to fill both roles, but they mean fundamentally
different things. `MVCCAbortTxnOp` is then given back its ability to delete
transactions from the queue directly without worrying about their reference
count. Next, the `unresolvedIntentQueue` is extended to be a little more
flexible. Instead of asserting that reference counts never drop below zero, it just
ignores changes that would make reference counts negative. This has the potential
to "leak" reference counts that saturate at zero and are then incremented again,
but we know that this will only ever happen for aborted transactions, which can
always be pushed again to rediscover that they have been aborted.

Release note: None
nvb added a commit to nvb/cockroach that referenced this pull request Mar 19, 2019
Fixes cockroachdb#35816.

The fix in cockroachdb#35777 inadvertently broke `txnPushAttempt` and its interaction with
transactions that were found to be aborted. This commit restores this handling
by reverting part of the assertion added in cockroachdb#35772 and splitting up the handling
of "aborted intents" from the handling of "aborted transactions".

Aborted transactions were always one of the most tricky situations to handle in
rangefeed. The challenge is that rangefeed may be tracking a transaction that
has been abandoned and may want to push it to unblock its resolved timestamp,
but it doesn't have a way to know where the aborted transaction's intents live
on its range. The aborted transaction record can't tell us this either. So the
only thing rangefeed can do is push the transaction and convince itself that the
aborted intents on its range can be safely ignored. This poses a problem for
accurate reference counting of intents though. We need to be able to discard a
transaction and still handle the eventual resolution of its aborted intents.

To handle this properly, the change splits up `MVCCAbortIntentOp` into
`MVCCAbortIntentOp` and `MVCCAbortTxnOp`. Before this change, the single
operation type was attempting to fill both roles, but they mean fundamentally
different things. `MVCCAbortTxnOp` is then given back its ability to delete
transactions from the queue directly without worrying about their reference
count. Next, the `unresolvedIntentQueue` is extended to be a little more
flexible. Instead of asserting that reference counts never drop below zero, it just
ignores changes that would make reference counts negative. This has the potential
to "leak" reference counts that saturate at zero and are then incremented again,
but we know that this will only ever happen for aborted transactions, which can
always be pushed again to rediscover that they have been aborted.

Release note: None
nvb added a commit to nvb/cockroach that referenced this pull request Mar 19, 2019
Fixes cockroachdb#35816.

The fix in cockroachdb#35777 inadvertently broke `txnPushAttempt` and its interaction with
transactions that were found to be aborted. This commit restores this handling
by reverting part of the assertion added in cockroachdb#35772 and splitting up the handling
of "aborted intents" from the handling of "aborted transactions".

Aborted transactions were always one of the most tricky situations to handle in
rangefeed. The challenge is that rangefeed may be tracking a transaction that
has been abandoned and may want to push it to unblock its resolved timestamp,
but it doesn't have a way to know where the aborted transaction's intents live
on its range. The aborted transaction record can't tell us this either. So the
only thing rangefeed can do is push the transaction and convince itself that the
aborted intents on its range can be safely ignored. This poses a problem for
accurate reference counting of intents though. We need to be able to discard a
transaction and still handle the eventual resolution of its aborted intents.

To handle this properly, the change splits up `MVCCAbortIntentOp` into
`MVCCAbortIntentOp` and `MVCCAbortTxnOp`. Before this change, the single
operation type was attempting to fill both roles, but they mean fundamentally
different things. `MVCCAbortTxnOp` is then given back its ability to delete
transactions from the queue directly without worrying about their reference
count. Next, the `unresolvedIntentQueue` is extended to be a little more
flexible. Instead of asserting that reference counts never drop below zero, it just
ignores changes that would make reference counts negative. This has the potential
to "leak" reference counts that saturate at zero and are then incremented again,
but we know that this will only ever happen for aborted transactions, which can
always be pushed again to rediscover that they have been aborted.

Release note: None
craig bot pushed a commit that referenced this pull request Mar 19, 2019
35889: rangefeed: fix handling of aborted transactions found by txnPushAttempt r=nvanbenschoten a=nvanbenschoten

Fixes #35816.

The fix in #35777 inadvertently broke `txnPushAttempt` and its interaction with
transactions that were found to be aborted. This commit restores this handling
by reverting part of the assertion added in #35772 and splitting up the handling
of "aborted intents" from the handling of "aborted transactions".

Aborted transactions were always one of the most tricky situations to handle in
rangefeed. The challenge is that rangefeed may be tracking a transaction that
has been abandoned and may want to push it to unblock its resolved timestamp,
but it doesn't have a way to know where the aborted transaction's intents live
on its range. The aborted transaction record can't tell us this either. So the
only thing rangefeed can do is push the transaction and convince itself that the
aborted intents on its range can be safely ignored. This poses a problem for
accurate reference counting of intents though. We need to be able to discard a
transaction and still handle the eventual resolution of its aborted intents.

To handle this properly, the change splits up `MVCCAbortIntentOp` into
`MVCCAbortIntentOp` and `MVCCAbortTxnOp`. Before this change, the single
operation type was attempting to fill both roles, but they mean fundamentally
different things. `MVCCAbortTxnOp` is then given back its ability to delete
transactions from the queue directly without worrying about their reference
count. Next, the `unresolvedIntentQueue` is extended to be a little more
flexible. Instead of asserting that reference counts never drop below zero, it just
ignores changes that would make reference counts negative. This has the potential
to "leak" reference counts that saturate at zero and are then incremented again,
but we know that this will only ever happen for aborted transactions, which can
always be pushed again to rediscover that they have been aborted.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
nvb added a commit to nvb/cockroach that referenced this pull request Mar 21, 2019
Fixes cockroachdb#35816.

The fix in cockroachdb#35777 inadvertently broke `txnPushAttempt` and its interaction with
transactions that were found to be aborted. This commit restores this handling
by reverting part of the assertion added in cockroachdb#35772 and splitting up the handling
of "aborted intents" from the handling of "aborted transactions".

Aborted transactions were always one of the most tricky situations to handle in
rangefeed. The challenge is that rangefeed may be tracking a transaction that
has been abandoned and may want to push it to unblock its resolved timestamp,
but it doesn't have a way to know where the aborted transaction's intents live
on its range. The aborted transaction record can't tell us this either. So the
only thing rangefeed can do is push the transaction and convince itself that the
aborted intents on its range can be safely ignored. This poses a problem for
accurate reference counting of intents though. We need to be able to discard a
transaction and still handle the eventual resolution of its aborted intents.

To handle this properly, the change splits up `MVCCAbortIntentOp` into
`MVCCAbortIntentOp` and `MVCCAbortTxnOp`. Before this change, the single
operation type was attempting to fill both roles, but they mean fundamentally
different things. `MVCCAbortTxnOp` is then given back its ability to delete
transactions from the queue directly without worrying about their reference
count. Next, the `unresolvedIntentQueue` is extended to be a little more
flexible. Instead of asserting that reference counts never drop below zero, it just
ignores changes that would make reference counts negative. This has the potential
to "leak" reference counts that saturate at zero and are then incremented again,
but we know that this will only ever happen for aborted transactions, which can
always be pushed again to rediscover that they have been aborted.

Release note: None
@nvb nvb deleted the nvanbenschoten/betterAssert branch March 25, 2019 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants