slstorage: do not use InitPut with FailOnTombstones on Insert #138706
Conversation
This commit switches the usage of `InitPut` KV request when inserting a new sql liveness session from `FailOnTombstones=true` to `FailOnTombstones=false`. The contract of the `Insert` method is that the caller should never provide previously used session ID which is achieved by generating a random V4 UUID, so the probability of ever generating a duplicate is miniscule. The usage of `FailOnTombstones=true` option provides additional protection if the same session ID is reused within the GC TTL of the relevant range (because tombstones would get removed on MVCC garbage collection and/or compactions in pebble). This usage was introduced in 686f323 where we switched from SQL INSERT statement - which behaved similar to `FailOnTombstones=false`. My main motivation behind this change is that this is the only place where we use `FailOnTombstones=true` option of the InitPut, and we're about to deprecate the InitPuts in favor of CPuts, yet CPuts don't have this option which would make the deprecation process more involved. Also I think the current handling of this scenario by the code is partially broken. Namely, if previously used session ID is provided while there is a tombstone on the relevant key, the ConditionFailedError would be returned, and the loop in `Instance.createSession` would exceed the retry duration because on this error type we do not reset `session.id` field. This will result in not being able to create a session by the SQL instance, so it will fatal the process. On a quick search via Glean we never encountered this fatal before though. With the change in this commit in the case of inserting the previously used session ID the behavior is undefined (I think one possible scenario is that other SQL instances won't be able to see this new instance because they have cached the session ID in the deadSessions cache, so the new instance won't be used for distributed queries; there are probably other consequences too). Still, it seems reasonable to me to rely on randomly-generated UUIDs never repeating without additional - but only temporary - protection of `FailOnTombstones`. Release note: None
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
thanks for this investigation!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @yuzefovich)
pkg/sql/sqlliveness/slstorage/slstorage.go line 532 at r1 (raw file):
} v := encodeValue(expiration) batch.InitPut(k, &v, false /* failOnTombstones */)
super nit: comment mentioning that we're relying on UUID uniqueness may be useful, but also if this is changing to a CPut soon anyway, perhaps it's not necessary.
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/sqlliveness/slstorage/slstorage.go line 532 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
super nit: comment mentioning that we're relying on UUID uniqueness may be useful, but also if this is changing to a CPut soon anyway, perhaps it's not necessary.
Yeah, we'll switch this to CPut shortly, so I won't add a comment.
This commit switches the usage of
InitPutKV request when inserting a new sql liveness session fromFailOnTombstones=truetoFailOnTombstones=false. The contract of theInsertmethod is that the caller should never provide previously used session ID which is achieved by generating a random V4 UUID, so the probability of ever generating a duplicate is miniscule. The usage ofFailOnTombstones=trueoption provides additional protection if the same session ID is reused within the GC TTL of the relevant range (because tombstones would get removed on MVCC garbage collection and/or compactions in pebble). This usage was introduced in 686f323 where we switched from SQL INSERT statement - which behaved similar toFailOnTombstones=false.My main motivation behind this change is that this is the only place where we use
FailOnTombstones=trueoption of the InitPut, and we're about to deprecate the InitPuts in favor of CPuts, yet CPuts don't have this option which would make the deprecation process more involved.Also I think the current handling of this scenario by the code is partially broken. Namely, if previously used session ID is provided while there is a tombstone on the relevant key, the ConditionFailedError would be returned, and the loop in
Instance.createSessionwould exceed the retry duration because on this error type we do not resetsession.idfield. This will result in not being able to create a session by the SQL instance, so it will fatal the process. On a quick search via Glean we never encountered this fatal before though.With the change in this commit in the case of inserting the previously used session ID the behavior is undefined (I think one possible scenario is that other SQL instances won't be able to see this new instance because they have cached the session ID in the deadSessions cache, so the new instance won't be used for distributed queries; there are probably other consequences too).
Still, it seems reasonable to me to rely on randomly-generated UUIDs never repeating without additional - but only temporary - protection of
FailOnTombstones.Informs: #71074.
Epic: None
Release note: None