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

sql/kv: unexpected unique constraint violation on UPDATE #43928

Closed
nvanbenschoten opened this issue Jan 13, 2020 · 6 comments
Closed

sql/kv: unexpected unique constraint violation on UPDATE #43928

nvanbenschoten opened this issue Jan 13, 2020 · 6 comments
Assignees
Labels
A-kv-transactions Relating to MVCC and the transactional model. A-sql-executor SQL txn logic C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Jan 13, 2020

To reproduce:

roachprod create local -n 1
roachprod stage local release v19.2.2
roachprod start local

# apply patch from https://github.com/rohany/cockroach/commit/947e889136113c46a840c59afb472be25972d334
make bin/workload
bin/workload init bank --rows 10
bin/workload run bank --rows 10 --concurrency 2
# Will hit: Error: pq: duplicate key value (id)=(5) violates unique constraint "bank_id_key"

Expected:

UPDATE statements should not throw unique constraint violation errors.

Scope:

This does not reproduce on the v19.1 release binary or on master. It only reproduces on a v19.2 release binary (v19.2.0 and v19.2.2 tested so far).

It also reproduces regardless of the kv.transaction.parallel_commits_enabled cluster setting, meaning that parallel commits is not to blame.

Trace:

Scan /Table/53/1/{2-3}, /Table/53/1/{9-10}
querying next range at /Table/53/1/2
r36: sending batch 1 Scan to (n1,s1):1
querying next range at /Table/53/2/2/0
r33: sending batch 1 PushTxn to (n1,s1):1
querying next range at /Table/53/1/9
r33: sending batch 1 Scan to (n1,s1):1
fetched: /bank/primary/2/-546/payload -> /'initial-VNfyUJHfCmMeAUoTgoSVvnByDyvpHNPHDfVoNWdXBFQpwMOBgNVtNijyTjmecvFqyeLHlDbIBRrbCzSeiHWSLmWbhIvh'
Del /Table/53/2/2/0
Del /Table/53/1/2/-546/0
CPut /Table/53/1/2/50/0 -> /TUPLE/3:3:Bytes/initial-VNfyUJHfCmMeAUoTgoSVvnByDyvpHNPHDfVoNWdXBFQpwMOBgNVtNijyTjmecvFqyeLHlDbIBRrbCzSeiHWSLmWbhIvh
InitPut /Table/53/2/2/0 -> /BYTES/0xba
fetched: /bank/primary/9/0/payload -> /'initial-yebOGytNpTjrZWGwZjBYJtxekffbsIuqDwKSlhbtpnimeKKeOKDMsPtPgQgYWYdwHmNTuINQvtqlintFvZIFmyMzbZLx'
Del /Table/53/2/9/0
Del /Table/53/1/9/0/0
CPut /Table/53/1/9/-596/0 -> /TUPLE/3:3:Bytes/initial-yebOGytNpTjrZWGwZjBYJtxekffbsIuqDwKSlhbtpnimeKKeOKDMsPtPgQgYWYdwHmNTuINQvtqlintFvZIFmyMzbZLx
InitPut /Table/53/2/9/0 -> /BYTES/0x86fdac
querying next range at /Table/53/1/2/-546/0
querying next range at /Table/53/1/9/-596/0
r33: sending batch 1 CPut, 3 Del, 1 EndTxn, 2 InitPut to (n1,s1):1
r36: sending batch 1 CPut, 1 Del to (n1,s1):1
querying next range at /Table/53/1/2
querying next range at /Table/53/1/9
r33: sending batch 1 RefreshRng to (n1,s1):1
r36: sending batch 1 RefreshRng to (n1,s1):1
querying next range at /Table/53/1/2/-546/0
querying next range at /Table/53/1/9/-596/0
r33: sending batch 1 CPut, 3 Del, 1 EndTxn, 2 InitPut to (n1,s1):1
r36: sending batch 1 CPut, 1 Del to (n1,s1):1
execution failed after 0 rows: duplicate key value (id)=(2,-546) violates unique constraint "bank_id_key"
querying next range at /Table/53/2/2/0
r33: sending batch 1 EndTxn to (n1,s1):1
				SET tracing=on,kv,results;
				UPDATE bank
				SET balance = CASE id WHEN 9 THEN balance-596 WHEN 2 THEN balance+596 END
				WHERE id IN (9, 2);
				set tracing=off pq: duplicate key value (id)=(2,-546) violates unique constraint "bank_id_key"
Error: pq: duplicate key value (id)=(2,-546) violates unique constraint "bank_id_key"
@nvanbenschoten nvanbenschoten added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv-transactions Relating to MVCC and the transactional model. A-sql-executor SQL txn logic labels Jan 13, 2020
@nvanbenschoten
Copy link
Member Author

The bisect indicates that this was recently fixed by f6a4dc5.

@nvanbenschoten
Copy link
Member Author

Given that that's the fix, I suspect we were incorrectly handling a re-issued CPut that had previously succeeded after the successful RefreshRange.

@nvanbenschoten
Copy link
Member Author

Surprisingly, this isn't due to #35140.

@nvanbenschoten
Copy link
Member Author

nvanbenschoten commented Jan 13, 2020

I think I see it. f6a4dc5#diff-e6e4d9675c8a7912b5c54b2f4c460df6L181 looks wrong. The condition should be if index > 0 && index <= len(meta.IntentHistory) {, or just if index > 0 {. So this bug dates all the way back to a147f24#diff-e6e4d9675c8a7912b5c54b2f4c460df6R147,

@nvanbenschoten
Copy link
Member Author

That bug was also present in v19.1.0, so it's not clear to me why we aren't seeing the same issue there. Perhaps we're failing to refresh in these cases and falling back to a full txn restart. Regardless, we'll want to backport a patch to both release branches. I'll put something together.

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jan 13, 2020
Fixes cockroachdb#43928.

This commit fixes a bug in `MVCCMetadata.GetPrevIntentSeq` that was present
since its inception (a147f24). The method was failing to properly look up
the previous intent sequence when the last sequence in the sequence history
should have been returned. This could cause errors for read-write KV ops
like CPut, InitPut, and Increment.

This commit does not need to land on master because it was already fixed
there by f6a4dc5. It will need to end up on release-19.1 as well though.

Release note (bug fix): A SQL row write that is re-issued after
already succeeding will no longer throw a duplicate key error when
the previous write in its transaction deleted the row.
@nvanbenschoten
Copy link
Member Author

Fixed by #43937.

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jan 13, 2020
Fixes cockroachdb#43928.

This commit fixes a bug in `MVCCMetadata.GetPrevIntentSeq` that was present
since its inception (a147f24). The method was failing to properly look up
the previous intent sequence when the last sequence in the sequence history
should have been returned. This could cause errors for read-write KV ops
like CPut, InitPut, and Increment.

This commit does not need to land on master because it was already fixed
there by f6a4dc5. It will need to end up on release-19.1 as well though.

Release note (bug fix): A SQL row write that is re-issued after
already succeeding will no longer throw a duplicate key error when
the previous write in its transaction deleted the row.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-transactions Relating to MVCC and the transactional model. A-sql-executor SQL txn logic C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

2 participants