opt: remove uniqueness checks for gen_random_uuid() #61132
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)
pkg/sql/opt/exec/execbuilder/testdata/unique, line 1690 at r1 (raw file):
label: buffer 1 # We can also detect gen_random_uuid() when it is a projection.
Can you add a test where gen_random_uuid comes from a DEFAULT column expression?
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 216 at r1 (raw file):
// the columns has UUID type and returns gen_random_uuid(), unique check not // needed. requireChecksForGenRandUUID := UniquenessChecksForGenRandomUUIDClusterMode.Get(&mb.b.evalCtx.Settings.SV)
Super nit, but given that the default is false, we may as well check it after the other checks (so we can avoid the check in the common non-UUID case)
rytaft
force-pushed
the
uuid-elim
branch
2 times, most recently
from
1ef5583
to
67fb0e4
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/unique, line 1690 at r1 (raw file):
Previously, RaduBerinde wrote…
Can you add a test where gen_random_uuid comes from a DEFAULT column expression?
Done.
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 216 at r1 (raw file):
Previously, RaduBerinde wrote…
Super nit, but given that the default is false, we may as well check it after the other checks (so we can avoid the check in the common non-UUID case)
Done.
This commit removes uniqueness checks for UUID columns that are set to gen_random_uuid(), because uniqueness can be sufficiently guaranteed by randomness. The probability of collision when using gen_random_uuid() is extremely low. If users still want the checks, however, they can set sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled, a new cluster setting, to true. By default the setting is false. Fixes cockroachdb#57790 Release justification: This commit is a low-risk update to new functionality. Release note (sql change): Added a new cluster setting sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled, which controls creation of uniqueness checks for UUID columns set to gen_random_uuid(). When enabled, uniqueness checks will be added for the UUID column if it has a unique constraint that cannot be enforced by an index. When disabled, no uniqueness checks are planned for these columns, and uniqueness is assumed due to the near-zero collision probability of gen_random_uuid()
There was a problem hiding this comment.
bors r+
Reviewable status:
There was a problem hiding this comment.
Sorry for the late drive-by, but thought I'd mention that it might be nice for the docs to be clear what the non-obvious consequences of a collision are. For example, I think if you queried for the duplicate rows and the optimizer chose a locality optimized search, you'd only ever see 1 of them. There might be other weird side effects. But I could also not be respecting the low chances of a collision and overly paranoid...
Reviewed 2 of 6 files at r2.
Reviewable status:
|
Build succeeded: |
|
I think you're right that we should be sure to call this out in the docs. Added a docs-todo tag (cc @rmloveland) |
The chance of a civilization ending asteroid hitting the earth in your lifetime is about 1 in 10 million. That will happen 100 times before we ever find a duplicate UUID, even after having generated 103 trillion of them in the same table. |
|
^assuming a perfect random source, which is a tall order :) |
+1. I've seen enough bugs in random number generation to infer that the asteroid ending event isn't our biggest problem :-D |
This commit removes uniqueness checks for UUID columns that are set
to
gen_random_uuid(), because uniqueness can be sufficiently guaranteedby randomness. The probability of collision when using
gen_random_uuid()is extremely low. If users still want the checks, however, they can set
sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled, a new clustersetting, to true. By default the setting is false.
Fixes #57790
Release justification: This commit is a low-risk update to new functionality.
Release note (sql change): Added a new cluster setting
sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled, which controls
creation of uniqueness checks for UUID columns set to gen_random_uuid().
When enabled, uniqueness checks will be added for the UUID column if it
has a unique constraint that cannot be enforced by an index. When disabled,
no uniqueness checks are planned for these columns, and uniqueness is
assumed due to the near-zero collision probability of gen_random_uuid()