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

storage: no-op writes should not be proposed through Raft #23942

Closed
nvanbenschoten opened this issue Mar 16, 2018 · 0 comments · Fixed by #24345
Closed

storage: no-op writes should not be proposed through Raft #23942

nvanbenschoten opened this issue Mar 16, 2018 · 0 comments · Fixed by #24345
Assignees
@nvanbenschoten
Milestone
2.1

Comments

@nvanbenschoten
Copy link
Member

The meat of this issue was described in #20448 (comment).

Nowhere in the write cmd evaluation/proposal path do we ever filter out no-op commands or commands that decided that they didn't need to write anything during evaluation. Unless I'm missing something, this omission would be pretty catastrophic for this workload. The reason is that a contended workload will naturally result in waves of PushTxnRequests and ResolveIntentRequests. However, in most cases, no more than one of these requests should ever actually perform a write. The rest will usually find a transaction in whatever state it desires and should exit early. An example of this is a PushTxnRequests who finds its transaction in an ABORTED or COMMITTED state. This should be a quick no-op, but I think it will actually result in a consensus write. If each request in these waves actually performs a write, it's more understandable why HeartbeatTxnRequests are being starved in the CommandQueue.

We should change the write cmd evaluation/proposal path to detect no-op results and short circuit them before they are proposed through Raft.
@nvanbenschoten nvanbenschoten added this to the 2.1 milestone Mar 16, 2018
@nvanbenschoten nvanbenschoten self-assigned this Mar 16, 2018
This was referenced Mar 16, 2018
Closed
Closed
@tbg tbg mentioned this issue Mar 25, 2018
Closed
@bdarnell bdarnell mentioned this issue Mar 26, 2018
Merged
@nvanbenschoten nvanbenschoten mentioned this issue Mar 27, 2018
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 29, 2018
@nvanbenschoten
storage: don't require consensus for no-op requests
30dda78 
Fixes cockroachdb#23942.

During evaluation, a request can result in having no effect for a variety
of reasons. For instance, a PushTxnRequest may arrive at a transaction
record to find that it is already ABORTED, in which case there's nothing
for it to do.

Before this change, we were still requiring that these empty results be
proposed through Raft. This change fixes this by detecting empty results
and bypassing consensus completely.

Release note (performance improvement): Write requests that result in
no-ops are no longer proposed through Raft.
@nvanbenschoten nvanbenschoten mentioned this issue Mar 29, 2018
Merged
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 29, 2018
@nvanbenschoten
storage: don't require consensus for no-op requests
ce7f26e 
Fixes cockroachdb#23942.

During evaluation, a request can result in having no effect for a variety
of reasons. For instance, a PushTxnRequest may arrive at a transaction
record to find that it is already ABORTED, in which case there's nothing
for it to do.

Before this change, we were still requiring that these empty results be
proposed through Raft. This change fixes this by detecting empty results
and bypassing consensus completely.

Release note (performance improvement): Write requests that result in
no-ops are no longer proposed through Raft.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 29, 2018
@nvanbenschoten
storage: don't require consensus for no-op requests
6fcb3db 
Fixes cockroachdb#23942.

During evaluation, a request can result in having no effect for a variety
of reasons. For instance, a PushTxnRequest may arrive at a transaction
record to find that it is already ABORTED, in which case there's nothing
for it to do.

Before this change, we were still requiring that these empty results be
proposed through Raft. This change fixes this by detecting empty results
and bypassing consensus completely.

Release note (performance improvement): Write requests that result in
no-ops are no longer proposed through Raft.
craig bot added a commit that referenced this issue Mar 29, 2018
@craig
Merge #24309 #24345
d761da2 
24309: optbuilder: Fix bugs with subqueries, DISTINCT, and ORDER BY r=RaduBerinde a=rytaft

This commit fixes several miscellaneous issues.

1. Subqueries with aggregation and without a `FROM` clause were
   causing the outer query to incorrectly act as if it were in
   a grouping context. This commit fixes the issue by making
   sure the scope of the subquery is different than the scope
   of the outer query.
2. Queries with `DISTINCT` and `ORDER BY` in which the `ORDER BY`
   was ordering on an expression that was also part of the
   `SELECT` list were failing. This commit fixes the issue by
   reusing the existing projection for the `ORDER BY` column.
3. Several redundant columns were previously being synthesized
   for identical expressions. This commit fixes the issue by
   reusing existing columns wherever possible.

Release note: None

24345: storage: don't require consensus for no-op requests r=tschottdorf a=nvanbenschoten

Fixes #23942.

During evaluation, a request can result in having no effect for a variety
of reasons. For instance, a PushTxnRequest may arrive at a transaction
record to find that it is already ABORTED, in which case there's nothing
for it to do.

Before this change, we were still requiring that these empty results be
proposed through Raft. This change fixes this by detecting empty results
and bypassing consensus completely.

Release note (performance improvement): Write requests that result in
no-ops are no longer proposed through Raft.
@craig craig bot closed this as completed in #24345 Mar 29, 2018
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Mar 15, 2019
@nvanbenschoten
storage: don't require consensus for no-op requests
6e6b4f0 
Fixes cockroachdb#23942.

During evaluation, a request can result in having no effect for a variety
of reasons. For instance, a PushTxnRequest may arrive at a transaction
record to find that it is already ABORTED, in which case there's nothing
for it to do.

Before this change, we were still requiring that these empty results be
proposed through Raft. This change fixes this by detecting empty results
and bypassing consensus completely.

Release note (performance improvement): Write requests that result in
no-ops are no longer proposed through Raft.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvanbenschoten nvanbenschoten

Labels
None yet
Projects
None yet
Milestone
2.1
Development

Successfully merging a pull request may close this issue.

storage: don't require consensus for no-op requests nvanbenschoten/cockroach
1 participant
@nvanbenschoten