Fix for Issue:1731, drop table if exists may not result in schema change

[ashudeep-sharma-zz]

This Pull Request covers the fix for issue #1731,
Certain Statements like drop table if exists may not result in a schema change. This fix validates if the schema diff is zero and the query is dropStr, it shouldn;t throw an error in ApplySchema.

@michael-berlin , @alainjobart .Please check and let me know in case of any concerns.

---

This change is

[michael-berlin]

Thank you very much for this!

It looks good and I fully support this change.

I left a few comments. Can you please realize these? After that, I'll merge it. Thanks!

Previously, ashudeep-sharma (Ashu) wrote…

Fix for Issue:1731, drop table if exists may not result in schema change

This Pull Request covers the fix for issue #1731,

Certain Statements like drop table if exists may not result in a schema change. This fix validates if the schema diff is zero and the query is dropStr, it shouldn;t throw an error in ApplySchema.

@michael-berlin , @alainjobart .Please check and let me know in case of any concerns.

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

go/vt/schemamanager/tablet_executor.go, line 94 [r1] (raw file):

}

func (exec *TabletExecutor) getParsedDDLS(ctx context.Context, sqls []string) ([]*sqlparser.DDL, error) {

Please rename this to "parseDDLs" because it's actually not parsed yet and the method actively parses the statements.

get* implies that it's already parsed and stored as a field.

(FYI: Even if this was a field getter, Go discourages to use "get" as prefix for these because it's redundant. I.e. the method would be called parsedDDLs(). I understand that it's different in Java :) )

---

go/vt/schemamanager/tablet_executor.go, line 94 [r1] (raw file):

}

func (exec *TabletExecutor) getParsedDDLS(ctx context.Context, sqls []string) ([]*sqlparser.DDL, error) {

By turning this into a helper function, you can simplify its signature:

• make it a (static) function instead of a (class/struct) method: therefore remove "(exec *TabletExecutor)"
• remove the "ctx" parameter because it's not used

---

go/vt/schemamanager/tablet_executor.go, line 119 [r1] (raw file):

  }

  parsedDDLs, err := exec.getParsedDDLS(ctx,sqls)

You must always handle errors returned by functions.

A simple return in case of an error is fine here:

parsedDDLs, err := exec.getParsedDDLS(ctx,sqls)
if err != nil {
  return err
}

Note: The unit tests are currently failing because you don't return the error here. When you add the error check, they should succeed again.
FYI: You can run the unit tests as follows:

cd go/vt/schemamanager
go test

---

go/vt/schemamanager/tablet_executor.go, line 172 [r1] (raw file):

  }

  parsedDDLs, err := exec.getParsedDDLS(ctx,sqls)

Same comment as above:

A simple return in case of an error is missing here:

parsedDDLs, err := exec.getParsedDDLS(ctx,sqls)
if err != nil {
  return err
}

---

go/vt/schemamanager/tablet_executor.go, line 180 [r1] (raw file):

          "AfterSchema",
          schemaDiff.AfterSchema)
      if len(diffs) == 0 && parsedDDLs[i].Action!= sqlparser.DropStr{

nit: missing space before "!=" and "{". The precommit should have warned you about it and asked you to run "goimports" on the file first.

"goimports" will auto-format the file for you:

goimports -w go/vt/schemamanager/tablet_executor.go

When you use "GoClipse", you can also configure it to always run it on save.

---

go/vt/schemamanager/tablet_executor.go, line 180 [r1] (raw file):

          "AfterSchema",
          schemaDiff.AfterSchema)
      if len(diffs) == 0 && parsedDDLs[i].Action!= sqlparser.DropStr{

I would prefer to explicitly call out this exemption and add a comment for it.

In addition to that, I think we should only exempt it if it's a DROP IF EXISTS and not every DROP. I understand that the SQL Parser currently cannot tell you the difference. Therefore, it's fine to add a TODO for this for now.

Based on these comments, I propose the following change:

        if len(diffs) == 0 {
            if parsedDDLs[i].Action == sqlparser.DropStr {
                // DROP IF EXISTS on a nonexistent table does not change the schema. It's safe to ignore.
                // TODO(mberlin): Extend sqlparser to find out if the DDL contains IF EXISTS and check for it as well.
                continue
            }
            return fmt.Errorf("Schema change: '%s' does not introduce any table definition change.", sqls[i])
        }

---

test/schema.py, line 279 [r1] (raw file):

    self._check_tables(shard_1_master, 4)

  def test_schema_changes_drop_if_exists(self):

I suggest to rename it to test_schema_changes_drop_nonexistent_tables because it's more about testing the behavior of dropping nonexistent tables than DROP TABLE IF EXISTS specifically.

---

test/schema.py, line 280 [r1] (raw file):

  def test_schema_changes_drop_if_exists(self):
    self._apply_initial_schema()

I suggest to add a short comment here to explain what the test is supposed to test:

    """Tests the PreflightSchema logic for dropping nonexistent tables.

    If a table does not exist, DROP TABLE should error during preflight
    because the statement does not change the schema as there is
    nothing to drop.
    In case of DROP TABLE IF EXISTS though, it should not error as this
    is the MySQL behavior the user expects.
    """

---

test/schema.py, line 284 [r1] (raw file):

    self._check_tables(shard_1_master, 4)

    drop_table = ('DROP TABLE vt_select_test06;\n')

I suggest to use a more explicit table name here which will self-explain the fact that the table does not exist.

e.g.:

drop_table = ('DROP TABLE nonexistent_table;)

---

test/schema.py, line 284 [r1] (raw file):

    self._check_tables(shard_1_master, 4)

    drop_table = ('DROP TABLE vt_select_test06;\n')

nit: No "\n" necessary at the end.

---

test/schema.py, line 286 [r1] (raw file):

    drop_table = ('DROP TABLE vt_select_test06;\n')
    stdout = self._apply_schema(test_keyspace, drop_table, expect_fail=True)
    self.assertIn('Unknown table', ''.join(stdout))

Based on the code above, it looks like this fails in the actual operation and not in the preflight phase?

If that's the case, can you please add a TODO here? Feel free to tag it with my name.

# TODO(mberlin): This currently fails during the actual change.
# schemamanager should fail in preflight instead. Change the error message 
# we check for once schemamanager prevents this in the preflight phase.

---

test/schema.py, line 288 [r1] (raw file):

    self.assertIn('Unknown table', ''.join(stdout))

    #This Query may not result in schema change and should be allowed

nit: Missing space after "#".
nit: Missing period at the end. (Always try to use full sentences.)

---

test/schema.py, line 292 [r1] (raw file):

    self._apply_schema(test_keyspace, drop_if_exists)

  # check number of tables

nit: comment not correctly indented.

I suggest to remove it instead. The code is self-explanatory enough :)

---

Comments from Reviewable

[ashudeep-sharma-zz]

Review status: 0 of 2 files reviewed at latest revision, 13 unresolved discussions.

---

go/vt/schemamanager/tablet_executor.go, line 94 [r1] (raw file):

goimports -w go/vt/schemamanager/tablet_executor.go

---

go/vt/schemamanager/tablet_executor.go, line 94 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

By turning this into a helper function, you can simplify its signature:

• make it a (static) function instead of a (class/struct) method: therefore remove "(exec *TabletExecutor)"
• remove the "ctx" parameter because it's not used

Done.

---

go/vt/schemamanager/tablet_executor.go, line 119 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

You must always handle errors returned by functions.

A simple return in case of an error is fine here:

parsedDDLs, err := exec.getParsedDDLS(ctx,sqls)
if err != nil {
  return err
}

Note: The unit tests are currently failing because you don't return the error here. When you add the error check, they should succeed again.
FYI: You can run the unit tests as follows:

cd go/vt/schemamanager
go test

I tried fixing TestTabletExecutorExecute in tablet_executor_test.go, there seems to be a weird problem that i identified, the queries like "drop table" and "drop table if exists" corresponds to 2 rows in schemaDiff array in preflightSchemaChanges function, one of them being nil. I just want to checkout why is it happening that it results in 2 schemadiffs, ideally it should result in only one diff.

Also just wanted to checkout that we are not doing null check for schemaDiff in this piece of code:

for i, schemaDiff := range schemaDiffs {
diffs := tmutils.DiffSchemaToArray(
"BeforeSchema",
schemaDiff.BeforeSchema,
"AfterSchema",
schemaDiff.AfterSchema)
if len(diffs) == 0 {
if parsedDDLs[i].Action == sqlparser.DropStr {
// DROP IF EXISTS on a nonexistent table does not change the schema. It's safe to ignore.
// TODO(mberlin): Extend sqlparser to find out if the DDL contains IF EXISTS and check for it as well.
continue
}
return fmt.Errorf("Schema change: '%s' does not introduce any table definition change.", sqls[i])
}
}
Is it okay to go with this change? Let me know your thoughts and i will fix the tests.

---

go/vt/schemamanager/tablet_executor.go, line 172 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

Same comment as above:

A simple return in case of an error is missing here:

parsedDDLs, err := exec.getParsedDDLS(ctx,sqls)
if err != nil {
  return err
}

Done.

---

go/vt/schemamanager/tablet_executor.go, line 180 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

I would prefer to explicitly call out this exemption and add a comment for it.

In addition to that, I think we should only exempt it if it's a DROP IF EXISTS and not every DROP. I understand that the SQL Parser currently cannot tell you the difference. Therefore, it's fine to add a TODO for this for now.

Based on these comments, I propose the following change:

      if len(diffs) == 0 {
          if parsedDDLs[i].Action == sqlparser.DropStr {
              // DROP IF EXISTS on a nonexistent table does not change the schema. It's safe to ignore.
              // TODO(mberlin): Extend sqlparser to find out if the DDL contains IF EXISTS and check for it as well.
              continue
          }
          return fmt.Errorf("Schema change: '%s' does not introduce any table definition change.", sqls[i])
      }

Done.

---

go/vt/schemamanager/tablet_executor.go, line 180 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

nit: missing space before "!=" and "{". The precommit should have warned you about it and asked you to run "goimports" on the file first.

"goimports" will auto-format the file for you:

goimports -w go/vt/schemamanager/tablet_executor.go

When you use "GoClipse", you can also configure it to always run it on save.

Done.

---

test/schema.py, line 279 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

I suggest to rename it to test_schema_changes_drop_nonexistent_tables because it's more about testing the behavior of dropping nonexistent tables than DROP TABLE IF EXISTS specifically.

Done.

---

test/schema.py, line 280 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

I suggest to add a short comment here to explain what the test is supposed to test:

    """Tests the PreflightSchema logic for dropping nonexistent tables.

    If a table does not exist, DROP TABLE should error during preflight
    because the statement does not change the schema as there is
    nothing to drop.
    In case of DROP TABLE IF EXISTS though, it should not error as this
    is the MySQL behavior the user expects.
    """

Done.

---

test/schema.py, line 284 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

nit: No "\n" necessary at the end.

Done.

---

test/schema.py, line 284 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

I suggest to use a more explicit table name here which will self-explain the fact that the table does not exist.

e.g.:

drop_table = ('DROP TABLE nonexistent_table;)

Done.

---

test/schema.py, line 286 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

Based on the code above, it looks like this fails in the actual operation and not in the preflight phase?

If that's the case, can you please add a TODO here? Feel free to tag it with my name.

# TODO(mberlin): This currently fails during the actual change.
# schemamanager should fail in preflight instead. Change the error message 
# we check for once schemamanager prevents this in the preflight phase.

Done.

---

test/schema.py, line 288 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

nit: Missing space after "#".
nit: Missing period at the end. (Always try to use full sentences.)

Done.

---

test/schema.py, line 292 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

nit: comment not correctly indented.

I suggest to remove it instead. The code is self-explanatory enough :)

Done.

---

Comments from Reviewable

[michael-berlin]

Reviewed 1 of 2 files at r2.
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

go/vt/schemamanager/tablet_executor.go, line 119 [r1] (raw file):

Previously, ashudeep-sharma (Ashu) wrote…

I tried fixing TestTabletExecutorExecute in tablet_executor_test.go, there seems to be a weird problem that i identified, the queries like "drop table" and "drop table if exists" corresponds to 2 rows in schemaDiff array in preflightSchemaChanges function, one of them being nil. I just want to checkout why is it happening that it results in 2 schemadiffs, ideally it should result in only one diff.

Also just wanted to checkout that we are not doing null check for schemaDiff in this piece of code:

for i, schemaDiff := range schemaDiffs {
diffs := tmutils.DiffSchemaToArray(
"BeforeSchema",
schemaDiff.BeforeSchema,
"AfterSchema",
schemaDiff.AfterSchema)
if len(diffs) == 0 {
if parsedDDLs[i].Action == sqlparser.DropStr {
// DROP IF EXISTS on a nonexistent table does not change the schema. It's safe to ignore.
// TODO(mberlin): Extend sqlparser to find out if the DDL contains IF EXISTS and check for it as well.
continue
}
return fmt.Errorf("Schema change: '%s' does not introduce any table definition change.", sqls[i])
}
}
Is it okay to go with this change? Let me know your thoughts and i will fix the tests.

The nil error was my bad. I fixed it here: https://github.com//pull/1770

The test will keep failing because we changed the code. It will stop failing when the parser exposes an IF EXISTS flag to us and we extend the fake.

For now, I suggest to delete the test. The test were actually two, so I did split them: #1771

Please merge the master or rebase your branch to the latest master. Then you'll have the two changes.

After that, you can comment the TestTabletExecutorExecute_PreflightWithoutChangesIsAnError test and leave a TODO there that we can uncomment it when we improve that code.

---

Comments from Reviewable

[ashudeep-sharma-zz]

Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

go/vt/schemamanager/tablet_executor.go, line 119 [r1] (raw file):

Previously, michael-berlin (Michael Berlin) wrote…

The nil error was my bad. I fixed it here: #1770

The test will keep failing because we changed the code. It will stop failing when the parser exposes an IF EXISTS flag to us and we extend the fake.

For now, I suggest to delete the test. The test were actually two, so I did split them: #1771

Please merge the master or rebase your branch to the latest master. Then you'll have the two changes.

After that, you can comment the TestTabletExecutorExecute_PreflightWithoutChangesIsAnError test and leave a TODO there that we can uncomment it when we improve that code.

Done. Commented out the test and verified that the go test ran fine on my machine. I am a bit curious to know if its a bit involved task to parse If Exists clause from Drop Table, if not i can take that up, might need some initial points :).

---

Comments from Reviewable

[michael-berlin]

Previously, michael-berlin (Michael Berlin) wrote…

Thank you very much for this!

It looks good and I fully support this change.

I left a few comments. Can you please realize these? After that, I'll merge it. Thanks!

---

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

go/vt/schemamanager/tablet_executor.go, line 119 [r1] (raw file):

Previously, ashudeep-sharma (Ashu) wrote…

Done.
Commented out the test and verified that the go test ran fine on my machine. I am a bit curious to know if its a bit involved task to parse If Exists clause from Drop Table, if not i can take that up, might need some initial points :).

It's a bit of work. If you want to give it a try, here's what needs to be done:

Sugu already showed me what needs to be done for that.
(I'm currently busy with other stuff and won't be able to do it myself in the next weeks.)

• extend the parser. This can be based on what Sugu showed me here: https://github.com/youtube/vitess/compare/master...sougou:mberlin?expand=1
  => the parser change takes place in "go/vt/sqlparser/sql.y"
• once the parser is changed, the parser Go code (sql.go) needs to be regenerated:

cd go/vt/sqlparser/
make

• in ast.go you can see that Sugu added a new field "IfExists". After the parser code (sql.go) is regenerated, it will always fill in this field for you.
• That's all you need to use it in the preflight schema code

Since the parser changes, the unit tests for it must be updated as well. The unit test has already cases for DROP IF EXISTS.
https://github.com/youtube/vitess/blob/master/go/vt/sqlparser/parse_test.go#L515

The unit test compares against the output of the Format() function. You must extend that to output "if exists" as well and then output the test cases:
https://github.com/youtube/vitess/blob/dc4c86779742ba9df5f16d4023996c480b097809/go/vt/sqlparser/ast.go#L391

(Initially, Sugu did not support IF EXISTS on purpose in the parser because it was not relevant for other parts of Vitess. Therefore, the Format() function currently omits "if exists".)

---

Comments from Reviewable

[ashudeep-sharma-zz]

Thanks Michael for the details, i will give it a try in the next few days and will get back in case i run into issues.