Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zero autoinc mode #6749

Merged
merged 3 commits into from
Sep 24, 2020
Merged

Zero autoinc mode #6749

merged 3 commits into from
Sep 24, 2020

Conversation

MordFustang21
Copy link
Contributor

@MordFustang21 MordFustang21 commented Sep 19, 2020

This adds support to generate a sequence value when a 0 is passed instead of just null. This is the default behavior for MySQL.

Also need to look into detecting when sql_mode has been set to NO_AUTO_VALUE_ON_ZERO and handle that accordingly. I'm not sure the best way to go about that so any guidance there would be appreciated.

Ref #4751

This diff introduces -seq-auto-value-on-zero as a vtgate flag.

Until now sequences have only populated NULL columns, which is not consistent with
the default behavior of mysql. It's consistent with the NO_AUTO_VALUE_ON_ZERO sql mode.
In a perfect world we might want to detect per-keyspace sql modes. For now, it seems
simple and adequate to let people select at the vtgate level if they want this
behavior or not for all keyspace.

The motivation for this diff is that we have some situations where people still test code
against direct mysql without vtgate. We've twice had folks commit insert statements
with 0 for a sequence column that worked in a local test but failed against vtgate.
Aligning behavior of vtgate with mysql seems like the cleanest way to prevent that
in the future.

Signed-off-by: David Weitzman <dweitzman@pinterest.com>
Signed-off-by: David Weitzman <dweitzman@pinterest.com>
Signed-off-by: Derrick Laird <swampdonk@gmail.com>
@dweitzman
Copy link
Member

What's the thinking behind removing the option to opt-out of this and changing the behavior by default?

Backwards-incompatible changes seems to carry some risk, particularly with no option to opt into the old behavior. Seems like to do this by default without a flag there would have to be general consensus that this is a bug fix and not a behavior change. To me it's not obvious that this is a bug fix (vs a behavior change).

The stated policy for backwards-incompatible flag default value changes is that they flip during major version releases: https://github.com/vitessio/enhancements/blob/master/veps/vep-1.md#backwards-and-forwards-compatibility-promise

Seems like introducing a flag that's off by default in one major release and toggling the default in a future major release is probably the way to go?...

@sougou
Copy link
Contributor

sougou commented Sep 20, 2020

Looks like @dweitzman attempted this some time ago #4751. I voted for changing the behavior (without a flag) because it's the mysql default behavior. This is not likely to break anyone. I suggest that we just announce it in a RFC, which will make it to the release notes.

@dweitzman
Copy link
Member

Looks like @dweitzman attempted this some time ago #4751. I voted for changing the behavior (without a flag) because it's the mysql default behavior. This is not likely to break anyone. I suggest that we just announce it in a RFC, which will make it to the release notes.

Works for me. For our use case this is definitely an improvement in behavior

@sougou
Copy link
Contributor

sougou commented Sep 23, 2020

@MordFustang21 can you create an issue that announces this as a breaking change? Once that's done, we can merge this PR. Here's an example #6065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants