Allow zero (in) date when setting up internal _vt schema by shlomi-noach · Pull Request #12262 · vitessio/vitess

shlomi-noach · 2023-02-07T08:51:18Z

Description

Historically, there have been zero values in _vt.schema_migrations.requested_timestamp column; this was due to a bug, fixed in 3012060#diff-d7427b60825b46773b837642e539fd8f00382fc230b717043f3aa76d3ee7ae14R103

Unfortunately, those values violate the default sql_mode, which includes no_zero_date and no_zero_in_date.

If we make changes to _vt.schema_migrations table, via the schemadiff mechanism, and if zero values are found in requested_timestamp column, then those schema changes will fail, and the tablet will not be able to initialize properly.

In this PR, we take a general approach allowing zero in date and zero date in the session running the schema changes. This isn't specific to _vt.schema_migrations or to any specific column. It ensures we can preserve an already existing situation.

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2023-02-07T08:51:21Z

rohit-nayak-ps

Thanks for catching and fixing this!

lgtm

rohit-nayak-ps

A few unit tests need fixing to handle the new queries to get/set sql_mode.

shlomi-noach · 2023-02-07T12:20:08Z

v15 "backport": #12263 ; it's not a real backport because of how schema management has diverged, so it's its own fix.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-02-07T13:17:23Z

@rohit-nayak-ps what am I doing wrong with the tests here? My fix does not seem to produce the correct result. I'm getting a No such field in RowNamedValues

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

shlomi-noach · 2023-02-08T10:55:17Z

Thank you @rohit-nayak-ps for fixing the tests!

mattlord

LGTM! However, I think this should be address the more general issue of SQL_MODE in the context of what this package is supposed to achieve.

go/vt/sidecardb/sidecardb.go

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

mattlord

LGTM. Thank you! ❤️

I had a couple of minor comments that you can address as you see best.

mattlord · 2023-02-09T22:12:47Z

go/vt/sidecardb/sidecardb.go

+// We need to allow zero dates for existing sidecar database tables which may happen to
+// actually have zero (in) date values. setPermissiveSQLMode gets the current sql_mode, change it to a more relaxed value,
+// defer restoring it to the original value.


Suggestion:

// setPermissiveSQLMode gets the current sql_mode for the session, removes any // restrictions, and returns a function to restore it back to the original session value. // We need to allow for the recreation of any data that currently exists in the table, such // as e.g. allowing any zero dates that may already exist in a preexisting sidecar table.

mattlord · 2023-02-09T22:18:27Z

go/vt/sidecardb/sidecardb_test.go

+	sqlMode := sqltypes.MakeTestResult(sqltypes.MakeTestFields(
+		"sql_mode",
+		"varchar"),
+		"ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION",
+	)
+	db.AddQuery("select @@session.sql_mode as sql_mode", sqlMode)
+	db.AddQueryPattern("set @@session.sql_mode=.*", &sqltypes.Result{})


We shouldn't need this as it's done above in AddSchemaInitQueries().

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-02-12T12:45:36Z

unit tests still fail with "query not found"

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps · 2023-02-12T22:02:16Z

unit tests still fail with "query not found"

Fixed

shlomi-noach · 2023-02-13T11:34:04Z

Upgrade Downgrade Testing Query Serving (Schema) is flaky. Merging while it's failing.

vitess-bot · 2023-02-13T11:35:10Z

I was unable to backport this Pull Request to the following branches: release-16.0.

) * Allow zero (in) date when setting up internal _vt schema Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * modify test sto include @@sql_mod query support Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Allow zero (in) date when setting up internal _vt schema Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * modify test sto include @@sql_mod query support Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Fix test failures Signed-off-by: Rohit Nayak <rohit@planetscale.com> * Fix failing tests. Minor refactor Signed-off-by: Rohit Nayak <rohit@planetscale.com> * change sql_mode t omost permissive (empty) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * setPermissiveSQLMode Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * fixes per review Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Add missing mock query Signed-off-by: Rohit Nayak <rohit@planetscale.com> --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Signed-off-by: Rohit Nayak <rohit@planetscale.com> Co-authored-by: Rohit Nayak <rohit@planetscale.com>

…2406) * Allow zero (in) date when setting up internal _vt schema * modify test sto include @@sql_mod query support * Allow zero (in) date when setting up internal _vt schema * modify test sto include @@sql_mod query support * Fix test failures * Fix failing tests. Minor refactor * change sql_mode t omost permissive (empty) * setPermissiveSQLMode * fixes per review * Add missing mock query --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Signed-off-by: Rohit Nayak <rohit@planetscale.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Rohit Nayak <rohit@planetscale.com>

Allow zero (in) date when setting up internal _vt schema

aec3253

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach requested review from deepthi, harshit-gangal, rohit-nayak-ps and systay as code owners February 7, 2023 08:51

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Feb 7, 2023

rohit-nayak-ps approved these changes Feb 7, 2023

View reviewed changes

rohit-nayak-ps self-requested a review February 7, 2023 11:55

rohit-nayak-ps requested changes Feb 7, 2023

View reviewed changes

shlomi-noach added the Backport to: release-16.0 label Feb 7, 2023

shlomi-noach mentioned this pull request Feb 7, 2023

Bugfix/Backport to v15: Fix schema migrations requested_timestamp zero values #12263

Merged

3 tasks

modify test sto include @@sql_mod query support

cbef86e

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rohit-nayak-ps marked this pull request as draft February 7, 2023 13:50

rohit-nayak-ps self-assigned this Feb 7, 2023

rohit-nayak-ps force-pushed the internal-schemadiff-allow-zero-date branch from cbef86e to 1da859b Compare February 7, 2023 22:11

shlomi-noach and others added 4 commits February 7, 2023 23:12

Allow zero (in) date when setting up internal _vt schema

cb601fc

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

modify test sto include @@sql_mod query support

590f070

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Fix test failures

1da859b

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fix failing tests. Minor refactor

1bd8904

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps removed their assignment Feb 8, 2023

rohit-nayak-ps marked this pull request as ready for review February 8, 2023 09:26

rohit-nayak-ps approved these changes Feb 8, 2023

View reviewed changes

rohit-nayak-ps requested a review from mattlord February 8, 2023 11:51

mattlord requested changes Feb 8, 2023

View reviewed changes

go/vt/sidecardb/sidecardb.go Outdated Show resolved Hide resolved

go/vt/sidecardb/sidecardb.go Outdated Show resolved Hide resolved

shlomi-noach added 3 commits February 9, 2023 08:44

change sql_mode t omost permissive (empty)

73bc2ce

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

resolve conflict

444e39d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

setPermissiveSQLMode

2d9b25f

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rohit-nayak-ps mentioned this pull request Feb 9, 2023

Bug Report: Upgrading to 16.0.0-rc1 failed during PlannedReparentShard #12294

Closed

rohit-nayak-ps linked an issue Feb 9, 2023 that may be closed by this pull request

Bug Report: Upgrading to 16.0.0-rc1 failed during PlannedReparentShard #12294

Closed

mattlord self-requested a review February 9, 2023 22:05

mattlord approved these changes Feb 9, 2023

View reviewed changes

fixes per review

2b3864b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rohit-nayak-ps requested review from GuptaManan100 and frouioui as code owners February 12, 2023 21:07

rohit-nayak-ps force-pushed the internal-schemadiff-allow-zero-date branch from 251ef77 to e7986ee Compare February 12, 2023 21:11

Add missing mock query

e7986ee

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

shlomi-noach merged commit 32d77db into vitessio:main Feb 13, 2023

shlomi-noach deleted the internal-schemadiff-allow-zero-date branch February 13, 2023 11:34

frouioui mentioned this pull request Feb 21, 2023

Allow zero (in) date when setting up internal _vt schema (#12262) #12406

Merged

3 tasks

hmaurer mentioned this pull request Mar 21, 2024

oops #15542

Closed

Conversation

shlomi-noach commented Feb 7, 2023 • edited by mattlord Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot bot commented Feb 7, 2023 • edited by mattlord Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

Uh oh!

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

Uh oh!

shlomi-noach commented Feb 7, 2023

Uh oh!

shlomi-noach commented Feb 7, 2023

Uh oh!

shlomi-noach commented Feb 8, 2023

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattlord left a comment

Choose a reason for hiding this comment

Uh oh!

mattlord Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

shlomi-noach Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

mattlord Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

shlomi-noach Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

shlomi-noach commented Feb 12, 2023

Uh oh!

rohit-nayak-ps commented Feb 12, 2023

Uh oh!

shlomi-noach commented Feb 13, 2023

Uh oh!

vitess-bot bot commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shlomi-noach commented Feb 7, 2023 •

edited by mattlord

Loading

vitess-bot bot commented Feb 7, 2023 •

edited by mattlord

Loading