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.

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

client: retryable Txn errors should require acknowledgment from clients #22615

Closed
nvanbenschoten opened this issue Feb 12, 2018 · 5 comments · Fixed by #74563
Closed

client: retryable Txn errors should require acknowledgment from clients #22615

nvanbenschoten opened this issue Feb 12, 2018 · 5 comments · Fixed by #74563
Assignees
Labels
A-kv-client Relating to the KV client and the KV interface. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Feb 12, 2018

See #22337 (comment).

During retryable error handling, a client.Txn will reset its state to prepare for a retry internally before returning the error that triggered the retry. When doing this, the transaction will either increment its epoch or replace its ID. Either way, the transaction throws out all work it has previously performed*.
After this point, the new incarnation of the transaction can be used just like it is a new transaction.

This allows for abuses of db.Txn and txn.Exec, where a retry loop does not restart on a retryable error. For instance, in the following example, if the second txn.Put fails with a retryable error, the transaction will finish without either of the Put operations' results being committed.

err := db.Txn(func (ctx context.Context, txn *client.Txn) error {
  if err := txn.Put(ctx, "key", "val"); err != nil {
    return err
  }
  // Don't really care if this succeeds.
  _ := txn.Put(...)
  return nil
})

In order to avoid this type of misuse, we should require some kind of buy-in from users of client.Txn before a retried transaction can be used again. This could be as simple as requiring PrepareForRetry to be called before the next operation. This is already called in the db.Txn retry loop when it sees retryable Txn errors, so it should be invisible in almost all cases where client.Txn is being used correctly.

This will be addressed partially by spencerkimball@9decb16, which forces all txn aborted errors to propagate up to the db.Exec retry loop.

[*] this is not completely true, as a transaction with an incremented epoch will not need to write a new txn record and will still keep track of previous intents, but that is all transparent to users of client.Txn.

@nvanbenschoten nvanbenschoten added this to the 2.1 milestone Feb 12, 2018
@nvanbenschoten nvanbenschoten self-assigned this Feb 12, 2018
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 1, 2018
At the moment, parallel statement execution works by sending batches
concurrently through a single `client.Txn`. This make the handling of
retryable errors tricky because it's difficult to know when its safe
to prepare the transaction state for a retry. Our approach to this is
far from optimal, and relies on a mess of locking in both `client.Txn`
and `TxnCoordSender`. This works well enough to prevent anything from
seriously going wrong (cockroachdb#17197), but can result in some confounding error
behavior when statements operate in the context of transaction epochs
that they weren't expecting.

The ideal situation would be for all statements with a handle to a txn
to always work under the same txn epoch at a single point in time. Any
retryable error seen by these statements would be propagated up through
`client.Txn` without changing any state (and without yet being converted
to a `HandledRetryableTxnError`), and only after the statements have all
been synchronized would the retryable error be used to update the txn
and prepare for the retry attempt. This would require a change like cockroachdb#22615.
I've created a POC for this approach, but it is way to invasive to
cherry-pick.

So with our current state of things, we need to do a better job catching
errors caused by concurrent retries. In the past we've tried to carefully
determine which errors could be a symptom of a concurrent retry and ignore
them. I now think this was a mistake, as this process of inferring which
errors could be caused by a txn retry is fraught for failure. We now
always return retryable errors from synchronizeParallelStmts when they
exist. The reasoning for this is that if an error was a symptom of the
txn retry, it will not be present during the next txn attempt. If it was
not and instead was a legitimate query execution error, we expect to
hit it again on the next txn attempt and the behavior will mirror that
where the statement throwing the execution error was not even run before
the parallel queue hit the retryable error.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 1, 2018
At the moment, parallel statement execution works by sending batches
concurrently through a single `client.Txn`. This make the handling of
retryable errors tricky because it's difficult to know when its safe
to prepare the transaction state for a retry. Our approach to this is
far from optimal, and relies on a mess of locking in both `client.Txn`
and `TxnCoordSender`. This works well enough to prevent anything from
seriously going wrong (cockroachdb#17197), but can result in some confounding error
behavior when statements operate in the context of transaction epochs
that they weren't expecting.

The ideal situation would be for all statements with a handle to a txn
to always work under the same txn epoch at a single point in time. Any
retryable error seen by these statements would be propagated up through
`client.Txn` without changing any state (and without yet being converted
to a `HandledRetryableTxnError`), and only after the statements have all
been synchronized would the retryable error be used to update the txn
and prepare for the retry attempt. This would require a change like cockroachdb#22615.
I've created a POC for this approach, but it is way to invasive to
cherry-pick.

So with our current state of things, we need to do a better job catching
errors caused by concurrent retries. In the past we've tried to carefully
determine which errors could be a symptom of a concurrent retry and ignore
them. I now think this was a mistake, as this process of inferring which
errors could be caused by a txn retry is fraught for failure. We now
always return retryable errors from synchronizeParallelStmts when they
exist. The reasoning for this is that if an error was a symptom of the
txn retry, it will not be present during the next txn attempt. If it was
not and instead was a legitimate query execution error, we expect to
hit it again on the next txn attempt and the behavior will mirror that
where the statement throwing the execution error was not even run before
the parallel queue hit the retryable error.

Release note: None
@tbg tbg added the A-kv-client Relating to the KV client and the KV interface. label May 15, 2018
@tbg
Copy link
Member

tbg commented Jul 19, 2018

@andreimatei @nvanbenschoten is this still 2.1 material?

@nvanbenschoten
Copy link
Member Author

This is a part of @andreimatei's upcoming refactoring of the client.Txn api, so I'll defer to him on that.

@tbg tbg added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jul 22, 2018
@andreimatei andreimatei modified the milestones: 2.1, 2.2 Oct 4, 2018
@petermattis petermattis removed this from the 2.2 milestone Oct 5, 2018
benesch added a commit to benesch/cockroach that referenced this issue Dec 21, 2018
An optimizer code path could, in rare cases, fail to propagate a
transaction aborted error. This proved disastrous as, due to a footgun
in our transaction API (cockroachdb#22615), swallowing a transaction aborted error
results in proceeding with a brand new transaction that has no knowledge
of the earlier operations performed on the original transaction.

This presented as a rare and confusing bug in splits, as the split
transaction uses an internal executor. The internal executor would
occasionally silently return a new transaction that only had half of the
necessary operations performed on it, and committing that partial
transaction would result in a "range does not match splits" error.

Fixes cockroachdb#32784.

Release note: None
benesch added a commit to benesch/cockroach that referenced this issue Jan 2, 2019
An optimizer code path could, in rare cases, fail to propagate a
transaction aborted error. This proved disastrous as, due to a footgun
in our transaction API (cockroachdb#22615), swallowing a transaction aborted error
results in proceeding with a brand new transaction that has no knowledge
of the earlier operations performed on the original transaction.

This presented as a rare and confusing bug in splits, as the split
transaction uses an internal executor. The internal executor would
occasionally silently return a new transaction that only had half of the
necessary operations performed on it, and committing that partial
transaction would result in a "range does not match splits" error.

Fixes cockroachdb#32784.

Release note: None
benesch added a commit to benesch/cockroach that referenced this issue Jan 3, 2019
An optimizer code path could, in rare cases, fail to propagate a
transaction aborted error. This proved disastrous as, due to a footgun
in our transaction API (cockroachdb#22615), swallowing a transaction aborted error
results in proceeding with a brand new transaction that has no knowledge
of the earlier operations performed on the original transaction.

This presented as a rare and confusing bug in splits, as the split
transaction uses an internal executor. The internal executor would
occasionally silently return a new transaction that only had half of the
necessary operations performed on it, and committing that partial
transaction would result in a "range does not match splits" error.

Fixes cockroachdb#32784.

Release note: None
craig bot pushed a commit that referenced this issue Jan 3, 2019
33312: sql/opt: avoid swallowing TransactionAbortedErrors r=benesch a=benesch

An optimizer code path could, in rare cases, fail to propagate a
transaction aborted error. This proved disastrous as, due to a footgun
in our transaction API (#22615), swallowing a transaction aborted error
results in proceeding with a brand new transaction that has no knowledge
of the earlier operations performed on the original transaction.

This presented as a rare and confusing bug in splits, as the split
transaction uses an internal executor. The internal executor would
occasionally silently return a new transaction that only had half of the
necessary operations performed on it, and committing that partial
transaction would result in a "range does not match splits" error.

Fixes #32784.

Release note: None

/cc @tbg

33417: opt: Inline constant values r=andy-kimball a=andy-kimball

Inline constants in expressions like:

  SELECT x, x+1 FROM (VALUES (1)) AS t(x) ;

with the new inlining rules, this becomes:

  VALUES (1, 2)

The new inlining rules are useful for mutation expressions (e.g. UPDATE),
which can nest multiple Project and Values expressions that often use
constant values. For example:

  CREATE TABLE ab (a INT PRIMARY KEY, b INT AS (a + 1) STORED);
  UPDATE ab SET a=1

This now gets mapped by the optimizer to this internal equivalent:

  UPDATE ab SET a=1, b=2

Release note: None

33421: opt: Tighten up InlineProjectInProject rule r=andy-kimball a=andy-kimball

Allow inlining nested Project in case where there are duplicate refs
to same inner passthrough column. Previously, this case prevented
inlining. However, only duplicate references to inner *synthesized*
columns need to be detected.

Release note: None

Co-authored-by: Nikhil Benesch <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Jan 25, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Jan 31, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Feb 2, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Feb 8, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Feb 10, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Feb 10, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
lidorcarmel added a commit to lidorcarmel/cockroach that referenced this issue Feb 12, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
craig bot pushed a commit that referenced this issue Feb 15, 2022
74563: kv,kvcoord,sql: poison txnCoordSender after a retryable error r=lidorcarmel a=lidorcarmel

Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes #22615

Release note: None

74662: tsdb: expand mem per worker based on sql pool size r=dhartunian a=dhartunian

Previously, the memory limit for all `tsdb` workers was set at a static
64MiB. This cap created issues seen in #24018 where this limit was hit
on a 30 node cluster. To alleviate the issue, the number of workers was
reduced, raising the per-worker allocation.

We've currently hit this limit again as part of load testing with larger
clusters and have decided to make the per-query worker memory limit
dynamic. The per-worker limit is now raised based on the amount of memory
available to the SQL Pool via the `MemoryPoolSize` configuration
variable. This is set to be 25% of the system memory by default. The
`tsdb` memory cap per-worker is now doubled until it reaches `1/128` of
the memory pool setting.

For example, on a node with 128 - 256 GiB of memory, this will
correspond to 512 MiB allocated for all running `tsdb` queries.

In addition, the ts server is now connected to the same `BytesMonitor`
instance as the SQL memory monitor and workers will becapped at double
the query limit. Results are monitored as before but a cap is not
introduced there since we didn't have one present previously.

This behavior is gated behind a private cluster setting that's enabled
by default and sets the ratio at 1/128 of the SQL memory pool.

Resolves #72986

Release note (ops change): customers running clusters with 240 nodes or
more can effectively access tsdb metrics.

75677: randgen: add PopulateRandTable r=mgartner a=msbutler

PopulateRandTable populates the caller's table with random data. This helper
function aims to make it easier for engineers to develop randomized tests that
leverage randgen / sqlsmith.

Informs #72345

Release note: None

76334: opt: fix missing filters after join reordering r=mgartner a=mgartner

#### opt: add TES, SES, and rules to reorderjoins

This commit updates the output of the `reorderjoins` opt test command to
display the initial state of the `JoinOrderBuilder`. It adds additional
information to the output including the TES, SES, and conflict rules for
each edge.

Release note: None

#### opt: fix missing filters after join reordering

This commit eliminates logic in the `assoc`, `leftAsscom`, and
`rightAsscom` functions in the join order builder that aimed to prevent
generating "orphaned" predicates, where one or more referenced relations
are not in a join's input. In rare cases, this logic had the side effect
of creating invalid conflict rules for edges, which could prevent valid
predicates from being added to reordered join trees.

It is safe to remove these conditionals because they are unnecessary.
The CD-C algorithm already prevents generation of orphaned predicates by
checking that the total eligibility set (TES) is a subset of a join's
input vertices. In our implementation, this is handled by the
`checkNonInnerJoin` and `checkInnerJoin` functions.

Fixes #76522

Release note (bug fix): A bug has been fixed which caused the query optimizer
to omit join filters in rare cases when reordering joins, which could
result in incorrect query results. This bug was present since v20.2.


Co-authored-by: Lidor Carmel <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
@craig craig bot closed this as completed in c00ea84 Feb 15, 2022
RajivTS pushed a commit to RajivTS/cockroach that referenced this issue Mar 6, 2022
Previously kv users could lose parts of a transaction without getting an
error. After Send() returned a retryable error the state of txn got reset
which made it usable again. If the caller ignored the error they could
continue applying more operations without realizing the first part of the
transaction was discarded. See more details in the issue (cockroachdb#22615).

The simple case example is where the retryable closure of DB.Txn() returns
nil instead of returning the retryable error back to the retry loop - in this
case the retry loop declares success without realizing we lost the first part
of the transaction (all the operations before the retryable error).

This PR leaves the txn in a "poisoned" state after encountering an error, so
that all future operations fail fast. The caller is therefore expected to
reset the txn handle back to a usable state intentionally, by calling
Txn.PrepareForRetry(). In the simple case of DB.Txn() the retry loop will
reset the handle and run the retry even if the callback returned nil.

Closes cockroachdb#22615

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-client Relating to the KV client and the KV interface. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants