From bee009b9e3da0f10f795fb69d8e6183312f32660 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Wed, 10 Aug 2022 21:50:59 +0200 Subject: [PATCH] Allow zero in dates (#1161) * Merge pull request #31 from openark/zero-date Support zero date and zero in date, via dedicated command line flag * Merge pull request #32 from openark/existing-date-with-zero Support tables with existing zero dates * Remove un-needed ignore_versions file * Fix new lint errors from golang-ci update Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .github/workflows/golangci-lint.yml | 2 + doc/command-line-flags.md | 4 + go/base/context.go | 1 + go/cmd/gh-ost/main.go | 1 + go/logic/applier.go | 96 +++++++++++++++---- localtests/datetime-with-zero/create.sql | 20 ++++ localtests/datetime-with-zero/extra_args | 1 + .../existing-datetime-with-zero/create.sql | 21 ++++ .../existing-datetime-with-zero/extra_args | 1 + localtests/fail-datetime-with-zero/create.sql | 20 ++++ .../fail-datetime-with-zero/expect_failure | 1 + localtests/fail-datetime-with-zero/extra_args | 1 + .../fail-datetime-with-zero/ignore_versions | 1 + .../create.sql | 21 ++++ .../expect_failure | 1 + .../extra_args | 1 + 16 files changed, 172 insertions(+), 21 deletions(-) create mode 100644 localtests/datetime-with-zero/create.sql create mode 100644 localtests/datetime-with-zero/extra_args create mode 100644 localtests/existing-datetime-with-zero/create.sql create mode 100644 localtests/existing-datetime-with-zero/extra_args create mode 100644 localtests/fail-datetime-with-zero/create.sql create mode 100644 localtests/fail-datetime-with-zero/expect_failure create mode 100644 localtests/fail-datetime-with-zero/extra_args create mode 100644 localtests/fail-datetime-with-zero/ignore_versions create mode 100644 localtests/fail-existing-datetime-with-zero/create.sql create mode 100644 localtests/fail-existing-datetime-with-zero/expect_failure create mode 100644 localtests/fail-existing-datetime-with-zero/extra_args diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index caa1f57..07fa01d 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -19,3 +19,5 @@ jobs: - uses: actions/checkout@v3 - name: golangci-lint uses: golangci/golangci-lint-action@v3 + with: + version: v1.46.2 diff --git a/doc/command-line-flags.md b/doc/command-line-flags.md index 417255a..c689dfd 100644 --- a/doc/command-line-flags.md +++ b/doc/command-line-flags.md @@ -6,6 +6,10 @@ A more in-depth discussion of various `gh-ost` command line flags: implementatio Add this flag when executing on Aliyun RDS. +### allow-zero-in-date + +Allows the user to make schema changes that include a zero date or zero in date (e.g. adding a `datetime default '0000-00-00 00:00:00'` column), even if global `sql_mode` on MySQL has `NO_ZERO_IN_DATE,NO_ZERO_DATE`. + ### azure Add this flag when executing on Azure Database for MySQL. diff --git a/go/base/context.go b/go/base/context.go index e9dae69..f3fe712 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -92,6 +92,7 @@ type MigrationContext struct { AssumeRBR bool SkipForeignKeyChecks bool SkipStrictMode bool + AllowZeroInDate bool NullableUniqueKeyAllowed bool ApproveRenamedColumns bool SkipRenamedColumns bool diff --git a/go/cmd/gh-ost/main.go b/go/cmd/gh-ost/main.go index cc807cf..3a2052c 100644 --- a/go/cmd/gh-ost/main.go +++ b/go/cmd/gh-ost/main.go @@ -78,6 +78,7 @@ func main() { flag.BoolVar(&migrationContext.DiscardForeignKeys, "discard-foreign-keys", false, "DANGER! This flag will migrate a table that has foreign keys and will NOT create foreign keys on the ghost table, thus your altered table will have NO foreign keys. This is useful for intentional dropping of foreign keys") flag.BoolVar(&migrationContext.SkipForeignKeyChecks, "skip-foreign-key-checks", false, "set to 'true' when you know for certain there are no foreign keys on your table, and wish to skip the time it takes for gh-ost to verify that") flag.BoolVar(&migrationContext.SkipStrictMode, "skip-strict-mode", false, "explicitly tell gh-ost binlog applier not to enforce strict sql mode") + flag.BoolVar(&migrationContext.AllowZeroInDate, "allow-zero-in-date", false, "explicitly tell gh-ost binlog applier to ignore NO_ZERO_IN_DATE,NO_ZERO_DATE in sql_mode") flag.BoolVar(&migrationContext.AliyunRDS, "aliyun-rds", false, "set to 'true' when you execute on Aliyun RDS.") flag.BoolVar(&migrationContext.GoogleCloudPlatform, "gcp", false, "set to 'true' when you execute on a 1st generation Google Cloud Platform (GCP).") flag.BoolVar(&migrationContext.AzureMySQL, "azure", false, "set to 'true' when you execute on Azure Database on MySQL.") diff --git a/go/logic/applier.go b/go/logic/applier.go index 79d9083..d81f075 100644 --- a/go/logic/applier.go +++ b/go/logic/applier.go @@ -117,6 +117,24 @@ func (this *Applier) validateAndReadTimeZone() error { return nil } +// generateSqlModeQuery return a `sql_mode = ...` query, to be wrapped with a `set session` or `set global`, +// based on gh-ost configuration: +// - User may skip strict mode +// - User may allow zero dats or zero in dates +func (this *Applier) generateSqlModeQuery() string { + sqlModeAddendum := `,NO_AUTO_VALUE_ON_ZERO` + if !this.migrationContext.SkipStrictMode { + sqlModeAddendum = fmt.Sprintf("%s,STRICT_ALL_TABLES", sqlModeAddendum) + } + sqlModeQuery := fmt.Sprintf("CONCAT(@@session.sql_mode, ',%s')", sqlModeAddendum) + if this.migrationContext.AllowZeroInDate { + sqlModeQuery = fmt.Sprintf("REPLACE(REPLACE(%s, 'NO_ZERO_IN_DATE', ''), 'NO_ZERO_DATE', '')", sqlModeQuery) + } + sqlModeQuery = fmt.Sprintf("sql_mode = %s", sqlModeQuery) + + return sqlModeQuery +} + // readTableColumns reads table columns on applier func (this *Applier) readTableColumns() (err error) { this.migrationContext.Log.Infof("Examining table structure on applier") @@ -182,11 +200,33 @@ func (this *Applier) CreateGhostTable() error { sql.EscapeName(this.migrationContext.DatabaseName), sql.EscapeName(this.migrationContext.GetGhostTableName()), ) - if _, err := sqlutils.ExecNoPrepare(this.db, query); err != nil { - return err - } - this.migrationContext.Log.Infof("Ghost table created") - return nil + + err := func() error { + tx, err := this.db.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + sessionQuery := fmt.Sprintf(`SET SESSION time_zone = '%s'`, this.migrationContext.ApplierTimeZone) + sessionQuery = fmt.Sprintf("%s, %s", sessionQuery, this.generateSqlModeQuery()) + + if _, err := tx.Exec(sessionQuery); err != nil { + return err + } + if _, err := tx.Exec(query); err != nil { + return err + } + this.migrationContext.Log.Infof("Ghost table created") + if err := tx.Commit(); err != nil { + // Neither SET SESSION nor ALTER are really transactional, so strictly speaking + // there's no need to commit; but let's do this the legit way anyway. + return err + } + return nil + }() + + return err } // AlterGhost applies `alter` statement on ghost table @@ -201,11 +241,33 @@ func (this *Applier) AlterGhost() error { sql.EscapeName(this.migrationContext.GetGhostTableName()), ) this.migrationContext.Log.Debugf("ALTER statement: %s", query) - if _, err := sqlutils.ExecNoPrepare(this.db, query); err != nil { - return err - } - this.migrationContext.Log.Infof("Ghost table altered") - return nil + + err := func() error { + tx, err := this.db.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + sessionQuery := fmt.Sprintf(`SET SESSION time_zone = '%s'`, this.migrationContext.ApplierTimeZone) + sessionQuery = fmt.Sprintf("%s, %s", sessionQuery, this.generateSqlModeQuery()) + + if _, err := tx.Exec(sessionQuery); err != nil { + return err + } + if _, err := tx.Exec(query); err != nil { + return err + } + this.migrationContext.Log.Infof("Ghost table altered") + if err := tx.Commit(); err != nil { + // Neither SET SESSION nor ALTER are really transactional, so strictly speaking + // there's no need to commit; but let's do this the legit way anyway. + return err + } + return nil + }() + + return err } // AlterGhost applies `alter` statement on ghost table @@ -539,12 +601,9 @@ func (this *Applier) ApplyIterationInsertQuery() (chunkSize int64, rowsAffected return nil, err } defer tx.Rollback() + sessionQuery := fmt.Sprintf(`SET SESSION time_zone = '%s'`, this.migrationContext.ApplierTimeZone) - sqlModeAddendum := `,NO_AUTO_VALUE_ON_ZERO` - if !this.migrationContext.SkipStrictMode { - sqlModeAddendum = fmt.Sprintf("%s,STRICT_ALL_TABLES", sqlModeAddendum) - } - sessionQuery = fmt.Sprintf("%s, sql_mode = CONCAT(@@session.sql_mode, ',%s')", sessionQuery, sqlModeAddendum) + sessionQuery = fmt.Sprintf("%s, %s", sessionQuery, this.generateSqlModeQuery()) if _, err := tx.Exec(sessionQuery); err != nil { return nil, err @@ -1056,12 +1115,7 @@ func (this *Applier) ApplyDMLEventQueries(dmlEvents [](*binlog.BinlogDMLEvent)) } sessionQuery := "SET SESSION time_zone = '+00:00'" - - sqlModeAddendum := `,NO_AUTO_VALUE_ON_ZERO` - if !this.migrationContext.SkipStrictMode { - sqlModeAddendum = fmt.Sprintf("%s,STRICT_ALL_TABLES", sqlModeAddendum) - } - sessionQuery = fmt.Sprintf("%s, sql_mode = CONCAT(@@session.sql_mode, ',%s')", sessionQuery, sqlModeAddendum) + sessionQuery = fmt.Sprintf("%s, %s", sessionQuery, this.generateSqlModeQuery()) if _, err := tx.Exec(sessionQuery); err != nil { return rollback(err) diff --git a/localtests/datetime-with-zero/create.sql b/localtests/datetime-with-zero/create.sql new file mode 100644 index 0000000..526d1e6 --- /dev/null +++ b/localtests/datetime-with-zero/create.sql @@ -0,0 +1,20 @@ +drop table if exists gh_ost_test; +create table gh_ost_test ( + id int unsigned auto_increment, + i int not null, + dt datetime, + primary key(id) +) auto_increment=1; + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into gh_ost_test values (null, 7, '2010-10-20 10:20:30'); +end ;; diff --git a/localtests/datetime-with-zero/extra_args b/localtests/datetime-with-zero/extra_args new file mode 100644 index 0000000..0d60fb4 --- /dev/null +++ b/localtests/datetime-with-zero/extra_args @@ -0,0 +1 @@ +--allow-zero-in-date --alter="change column dt dt datetime not null default '1970-00-00 00:00:00'" diff --git a/localtests/existing-datetime-with-zero/create.sql b/localtests/existing-datetime-with-zero/create.sql new file mode 100644 index 0000000..5320d2c --- /dev/null +++ b/localtests/existing-datetime-with-zero/create.sql @@ -0,0 +1,21 @@ +set session sql_mode=''; +drop table if exists gh_ost_test; +create table gh_ost_test ( + id int unsigned auto_increment, + i int not null, + dt datetime not null default '1970-00-00 00:00:00', + primary key(id) +) auto_increment=1; + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into gh_ost_test values (null, 7, '2010-10-20 10:20:30'); +end ;; diff --git a/localtests/existing-datetime-with-zero/extra_args b/localtests/existing-datetime-with-zero/extra_args new file mode 100644 index 0000000..eb0e2ff --- /dev/null +++ b/localtests/existing-datetime-with-zero/extra_args @@ -0,0 +1 @@ +--allow-zero-in-date --alter="engine=innodb" diff --git a/localtests/fail-datetime-with-zero/create.sql b/localtests/fail-datetime-with-zero/create.sql new file mode 100644 index 0000000..526d1e6 --- /dev/null +++ b/localtests/fail-datetime-with-zero/create.sql @@ -0,0 +1,20 @@ +drop table if exists gh_ost_test; +create table gh_ost_test ( + id int unsigned auto_increment, + i int not null, + dt datetime, + primary key(id) +) auto_increment=1; + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into gh_ost_test values (null, 7, '2010-10-20 10:20:30'); +end ;; diff --git a/localtests/fail-datetime-with-zero/expect_failure b/localtests/fail-datetime-with-zero/expect_failure new file mode 100644 index 0000000..79356a1 --- /dev/null +++ b/localtests/fail-datetime-with-zero/expect_failure @@ -0,0 +1 @@ +Invalid default value for 'dt' diff --git a/localtests/fail-datetime-with-zero/extra_args b/localtests/fail-datetime-with-zero/extra_args new file mode 100644 index 0000000..9b72ac2 --- /dev/null +++ b/localtests/fail-datetime-with-zero/extra_args @@ -0,0 +1 @@ +--alter="change column dt dt datetime not null default '1970-00-00 00:00:00'" diff --git a/localtests/fail-datetime-with-zero/ignore_versions b/localtests/fail-datetime-with-zero/ignore_versions new file mode 100644 index 0000000..b6de5f8 --- /dev/null +++ b/localtests/fail-datetime-with-zero/ignore_versions @@ -0,0 +1 @@ +(5.5|5.6) diff --git a/localtests/fail-existing-datetime-with-zero/create.sql b/localtests/fail-existing-datetime-with-zero/create.sql new file mode 100644 index 0000000..5320d2c --- /dev/null +++ b/localtests/fail-existing-datetime-with-zero/create.sql @@ -0,0 +1,21 @@ +set session sql_mode=''; +drop table if exists gh_ost_test; +create table gh_ost_test ( + id int unsigned auto_increment, + i int not null, + dt datetime not null default '1970-00-00 00:00:00', + primary key(id) +) auto_increment=1; + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into gh_ost_test values (null, 7, '2010-10-20 10:20:30'); +end ;; diff --git a/localtests/fail-existing-datetime-with-zero/expect_failure b/localtests/fail-existing-datetime-with-zero/expect_failure new file mode 100644 index 0000000..79356a1 --- /dev/null +++ b/localtests/fail-existing-datetime-with-zero/expect_failure @@ -0,0 +1 @@ +Invalid default value for 'dt' diff --git a/localtests/fail-existing-datetime-with-zero/extra_args b/localtests/fail-existing-datetime-with-zero/extra_args new file mode 100644 index 0000000..31bc479 --- /dev/null +++ b/localtests/fail-existing-datetime-with-zero/extra_args @@ -0,0 +1 @@ +--alter="engine=innodb"