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

vtgate engine: command line flag to generate sequences for 0 columns #4751

Closed
wants to merge 3 commits into from

Conversation

dweitzman
Copy link
Member

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.

@dweitzman dweitzman requested a review from sougou as a code owner March 25, 2019 19:05
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>
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead just support that set syntax, and store the setting in the Session variable?

@dweitzman
Copy link
Member Author

Should we instead just support that set syntax, and store the setting in the Session variable?

I kinda think the best thing to do is make this the default and only behavior rather than supporting both. I'm having trouble imagining a real scenario where someone would want to enable NO_AUTO_VALUE_ON_ZERO. Seems like if push comes to shove in some kind of odd emergency where it's super important to insert a row with ID 0 you could always temporarily disable sequences or have a dba bypass vitess or for the moment you could use v1 or v2 destinations to bypass sequences.

If both modes were supported by vtgate, I don't see much value in being able to switch between them dynamically. Even if switching were supported, I wouldn't want to require all db connections to explicitly and individually opt-in to the normal default mysql behavior.

@sougou
Copy link
Contributor

sougou commented Mar 31, 2019

If this is the mysql default, I'm fine with changing behavior without a flag.
For good measure, we could intercept the set call and return an 'unsupported' error if they try to change it away from the default.

@sougou
Copy link
Contributor

sougou commented Aug 17, 2019

@dweitzman ping

@derekperkins
Copy link
Member

derekperkins commented Jul 7, 2020

We're interested in this and @MordFustang21 could take it over if @dweitzman doesn't have time. @sougou Do you feel differently about this now? Do you want the SET command intercepted or should we wait for those PRs to get merged?

@dweitzman
Copy link
Member Author

We're interested in this and @MordFustang21 could take it over if @dweitzman doesn't have time. @sougou Do you feel differently about this now? Do you want the SET command intercepted or should we wait for those PRs to get merged?

Feel free to take it over. It's a small diff. (One reason I never finished this was that I was feeling conflicted about how session variables should be handled)

Signed-off-by: David Weitzman <dweitzman@pinterest.com>
@MordFustang21 MordFustang21 mentioned this pull request Sep 19, 2020
@sougou
Copy link
Contributor

sougou commented Oct 31, 2020

@harshit-gangal @systay looks like this can be closed now?

@harshit-gangal
Copy link
Member

Closing in favour of #6749

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

Successfully merging this pull request may close these issues.

4 participants