kv: stop sending InitPut, replace with CPut #88519
Conversation
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/deleteInitPut
branch
from
faa1492 to
394c2aa
Compare
First half of cockroachdb#71074. This commit replaces all client-side use of InitPut request with the equivalent use of CPut. It uses the following equivalence to perform this mapping: ``` InitPut(key, value) -> CPutAllowingIfNotExists(key, value, value) ``` A future change in v23.2 (or do we now allow skipping one major version?) can remove the server-side handling of InitPut and remove the proto message entirely, once we no longer need to ensure mixed-version compatibility with v22.2. This is primarily a clean-up that reduces the KV API surface area. However, it's also a useful simplification for cockroachdb#72614. Release justification: None. Release note: None.
nvanbenschoten
force-pushed
the
nvanbenschoten/deleteInitPut
branch
from
394c2aa to
f7f014d
Compare
|
Is this abandoned? |
|
It's stale, but I'm happy to rebase it. Just needed a reviewer for the caller changes. |
| @@ -69,7 +70,11 @@ func CreateSystemTableInTxn( | |||
| b.CPut(tKey, desc.GetID(), nil) | |||
| b.CPut(catalogkeys.MakeDescMetadataKey(codec, desc.GetID()), desc.DescriptorProto(), nil) | |||
| if desc.IsSequence() { | |||
| b.InitPut(codec.SequenceKey(uint32(desc.GetID())), desc.GetSequenceOpts().Start, false /* failOnTombstones */) | |||
| // TODO DURING REVIEW: @ajwerner, why was this an InitPut instead of a | |||
There was a problem hiding this comment.
Does anyone on @cockroachdb/sql-schema have ideas about this?
| // NOTE: InitPutRequest is now deprecated and should no longer be used. See | ||
| // #71074. The request type still exists for compatibility with v22.2 and | ||
| // earlier releases. The server-side handling code can be deleted once | ||
| // compatibility with v22.2 is no longer needed. |
There was a problem hiding this comment.
nit: I think the "v22.2" references here should now be "v23.1".
| @@ -222,24 +222,24 @@ query T kvtrace | |||
| INSERT INTO t VALUES (5, 4, 'bar') | |||
| ---- | |||
| CPut /Table/112/1/5/0 -> /TUPLE/2:2:Int/4/1:3:Bytes/bar | |||
| InitPut /Table/112/2/4/5/0 -> /BYTES/ | |||
There was a problem hiding this comment.
It's not obvious to me why these were InitPuts in the first place - these secondary index keys should not already exist. Was there a performance benefit to only using CPut for the primary index to catch conflicts? Does the same benefit still exist for CPut vs CPutAllowingIfNotExists? If so, should we differentiate between the two types of CPuts int traces like this? And if there is no benefit toCPut over CPutAllowingIfNotExists, should we convert these secondary index puts into CPuts?
|
This PR was superseded by #138707. |
First half of #71074.
This commit replaces all client-side use of InitPut request with the equivalent use of CPut. It uses the following equivalence to perform this mapping:
A future change in v23.2 (or do we now allow skipping one major version?) can remove the server-side handling of InitPut and remove the proto message entirely, once we no longer need to ensure mixed-version compatibility with v22.2.
This is primarily a clean-up that reduces the KV API surface area. However, it's also a useful simplification for #72614.
Release justification: None.
Release note: None.