sql: add version gate to regrole type#70531
Conversation
|
This change is |
9115e15 to
24d2ece
Compare
nvb
left a comment
There was a problem hiding this comment.
Thanks!
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RichardJCai)
pkg/sql/types/minimum_type_version.go, line 16 at r1 (raw file):
// minimumTypeUsageVersions defines the minimum version needed for a new // data type.
nit: consider adding a comment here advising people not to remove this map and the IsTypeSupportedInVersion function even if it ever drops to being empty temporarily.
|
Let's just piggy-back on some other version added in this cycle. I don't think we need to add a new one. |
Ack, does it make sense to piggy back off Added in: |
ajwerner
left a comment
There was a problem hiding this comment.
Fine by me. How about we add a const in the sql package that has a useful name and is equal to the clusterversion key you choose. That can be a good place to anchor the comment. Then the version check will look like it makes sense.
Reviewable status:
24d2ece to
300bdd9
Compare
RichardJCai
left a comment
There was a problem hiding this comment.
Added an alias for MarkerDataKeysRegistry in the minimum_type_version file.
Reviewable status:
rafiss
left a comment
There was a problem hiding this comment.
lgtm after fixing html docs!
| <tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr> | ||
| <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'). Only one tracer can be configured at a time.</td></tr> | ||
| <tr><td><code>version</code></td><td>version</td><td><code>21.1-1164</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr> | ||
| <tr><td><code>version</code></td><td>version</td><td><code>21.1-1166</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr> |
There was a problem hiding this comment.
i think these docs changes should be reverted now
300bdd9 to
9cae889
Compare
Without the version gate, in a mixed cluster setting, creating a table on the new binary version with type REGROLE can cause errors when inserting on the old verison. Errors can also happen on schema changes where the old binary uses the new type. Release justification: GA blocker, without a version gate this can cause panics. Release note: None. Not in production.
9cae889 to
ee90182
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
|
TFTR! |
|
Build succeeded: |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/types/minimum_type_version.go, line 16 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: consider adding a comment here advising people not to remove this map and the
IsTypeSupportedInVersionfunction even if it ever drops to being empty temporarily.
@RichardJCai @nvanbenschoten Just stumbled across this code and wondering why we have this recommendation to not remove the map even it becomes empty? Is it so that in the future, when we will need a similar version gate, we already have the necessary infrastructure?
Yeah that is exactly why, so we have the necessary infrastructure. |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/types/minimum_type_version.go, line 16 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Yeah that is exactly why, so we have the necessary infrastructure.
I see, thanks. I'll probably remove it though in #77260 in order to break the dependency of sql/types on roachpb (via clusterversion). Do you have strong feelings about it?
Without the version gate, in a mixed cluster setting, creating
a table on the new binary version with type REGROLE can cause
errors when inserting on the old verison.
Errors can also happen on schema changes where the old binary uses
the new type.
Release justification: GA blocker, without a version gate this can
cause panics.
Release note: None. Not in production.