kvclient: serve some DeleteRequest's from write buffer

[stevendanna]

In order to ensure that a DELETE returns the correct number of deleted rows, it must be integrated with the write buffer so that it observes any previous deletes or writes.

Note that all DELETE statements are not yet handled since we don't yet have DeleteRange support.

Epic: none
Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

Another nice find!

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2)

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 429 at r1 (raw file):

			// what the GetResponse that we might have sent says.
			//
			// TODO(review): If we didn't serve the request and we also haven't been

Hm, it's an interesting question. I reviewed the current usage of DeleteRequest in the SQL land, and the only place where we do care about which keys were deleted (meaning we care about FoundKey boolean) is in deleteRangeNode.deleteSpans where we do set MustAcquireExclusiveLock flag.

In principle, I think it'd be cleaner if we added another boolean to DeleteRequest to indicate that we need to know precise value of FoundKey; in practice I think we can just adjust the contract of existing MustAcquireExclusiveLock to mention that if it's not set, then FoundKey in the response might be incorrect and leave a TODO to improve this.

---

pkg/sql/logictest/testdata/logic_test/buffered_writes line 6 at r1 (raw file):

SET kv_transaction_buffered_writes_enabled=true

statement no-rewrite ok

The way to disable column family mutator is to explicitly specify the column families in the DDL, so let's do that instead and remove new no-rewrite directive.

---

pkg/sql/logictest/testdata/logic_test/buffered_writes line 37 at r1 (raw file):

DELETE FROM t1 WHERE pk = 1

# The second delete be served from the write buffer

nit: s/be server/should be served/ here and below.

[stevendanna]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 429 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Hm, it's an interesting question. I reviewed the current usage of DeleteRequest in the SQL land, and the only place where we do care about which keys were deleted (meaning we care about FoundKey boolean) is in deleteRangeNode.deleteSpans where we do set MustAcquireExclusiveLock flag.

In principle, I think it'd be cleaner if we added another boolean to DeleteRequest to indicate that we need to know precise value of FoundKey; in practice I think we can just adjust the contract of existing MustAcquireExclusiveLock to mention that if it's not set, then FoundKey in the response might be incorrect and leave a TODO to improve this.

Thanks for taking a look. I've added a TODO on MustAcquireExclusiveLock and changed this to a note.

---

pkg/sql/logictest/testdata/logic_test/buffered_writes line 6 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

The way to disable column family mutator is to explicitly specify the column families in the DDL, so let's do that instead and remove new no-rewrite directive.

Aha, thanks. Done.

---

pkg/sql/logictest/testdata/logic_test/buffered_writes line 37 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/be server/should be served/ here and below.

Thanks. Done.

[yuzefovich]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @michae2)

[stevendanna]

bors r=yuzefovich

@arulajmani Feel free to still review. Happy to handle stuff in a follow-up. Just want to start reducing the delta between master and what we need to test end-to-end.

[craig]

Build succeeded:

• linux_amd64_build
• acceptance
• lint
• docker_image_amd64
• unit_tests
• examples_orms
• local_roachtest_fips
• local_roachtest
• check_generated_code
• linux_amd64_fips_build