Support ON UPDATE ddl statements

[EmmaLouise2018]

Bug Report

1. What did you do?

Looking at the ddl tests in the testDDLSuite, I was under the impression from TestDDLOnUpdateRestore that parsing sql statements with "ON UPDATE" were supported, however, in attempting to parse the following statement I encountered an error.

Code snippet:

 p := parser.New()
 testSQL := "create table test(id int key, FOREIGN KEY (id) REFERENCES test2(id) ON UPDATE CASCADE);"
 if _, _, err := p.Parse(testSQL, "", ""); err != nil {
     logger.Error("failed to parse", zap.Error(err))
 } 

2. What did you expect to see?

I expected this statement to be able to be parsed without an issue.

3. What did you see instead?

I encountered the error "error":"line 1 column 77 near \"UPDATE CASCADE);\"

Looking closer at the current tests in TestDDLOnUpdateRestore

func (ts *testDDLSuite) TestDDLOnUpdateRestore(c *C) {
    testCases := []NodeRestoreTestCase{
        {"ON UPDATE RESTRICT", "ON UPDATE RESTRICT"},
        {"on update CASCADE", "ON UPDATE CASCADE"},
        {"on update SET NULL", "ON UPDATE SET NULL"},
        {"on update no action", "ON UPDATE NO ACTION"},
    }
    extractNodeFunc := func(node Node) Node {
        return node.(*CreateTableStmt).Constraints[1].Refer.OnUpdate
    }
    RunNodeRestoreTest(c, testCases, "CREATE TABLE child ( id INT, parent_id INT, INDEX par_ind (parent_id), FOREIGN KEY (parent_id) REFERENCES parent(id)  ON DELETE CASCADE %s )", extractNodeFunc)
}

All ON UPDATE test cases are inserted after an ON DELETE CASCADE clause. When I removed, the ON DELETE and instead used

RunNodeRestoreTest(c, testCases, "CREATE TABLE child ( id INT, parent_id INT, INDEX par_ind (parent_id), FOREIGN KEY (parent_id) REFERENCES parent(id) %s )", extractNodeFunc)

The tests for TestDDLOnUpdateRestore start to fail

----------------------------------------------------------------------                                                                                                                   
FAIL: ddl_test.go:91: testDDLSuite.TestDDLOnUpdateRestore                                                                                                                                
                                                                                                                                                                                         
ddl_test.go:101:                                                                                                                                                                         
    RunNodeRestoreTest(c, testCases, "CREATE TABLE child ( id INT, parent_id INT, INDEX par_ind (parent_id), FOREIGN KEY (parent_id) REFERENCES parent(id) %s )", extractNodeFunc)       
util_test.go:113:                                                                                                                                                                        
    c.Assert(err, IsNil, comment)                                                                                                                                                        
... value *errors.withStack = line 1 column 126 near "UPDATE RESTRICT )"  ("line 1 column 126 near \"UPDATE RESTRICT )\" ")                                                              
... source ast_test.NodeRestoreTestCase{sourceSQL:"ON UPDATE RESTRICT", expectSQL:"ON UPDATE RESTRICT"}                                                                                  

Are DDL statements like create table test(id int key, FOREIGN KEY (id) REFERENCES test2(id) ON UPDATE CASCADE); not supported?

4. What version of TiDB SQL Parser are you using?

My go.mod file looks like this:

 github.com/pingcap/parser v0.0.0-20190312024907-3f6280b08c8b
 github.com/pingcap/tidb v0.0.0-20181120082053-821af9e9f65b9901bcac1661f28c41985697b511

[EmmaLouise2018]

Looking at parser.y https://github.com/pingcap/parser/blob/master/parser.y#L1771-L1806

ReferDef:
	"REFERENCES" TableName '(' IndexColNameList ')' OnDeleteOpt OnUpdateOpt

I believe this bug may stem from the fact we seem to rely on a DELETE option always being present before an UPDATE option is seen, which is not always the case in SQL.

[EmmaLouise2018]

I seemingly quick but probably naive way to fix this would be:

diff --git a/parser.y b/parser.y
index 793a1ec..602b293 100644
--- a/parser.y
+++ b/parser.y
@@ -1786,6 +1786,23 @@ ReferDef:
                        OnUpdate: onUpdateOpt,
                }
        }
+|      "REFERENCES" TableName '(' IndexColNameList ')' OnUpdateOpt OnDeleteOpt
+       {
+               var onDeleteOpt *ast.OnDeleteOpt
+               if $7 != nil {
+                       onDeleteOpt = $7.(*ast.OnDeleteOpt)
+               }
+               var onUpdateOpt *ast.OnUpdateOpt
+               if $6 != nil {
+                       onUpdateOpt = $6.(*ast.OnUpdateOpt)
+               }
+               $$ = &ast.ReferenceDef{
+                       Table: $2.(*ast.TableName),
+                       IndexColNames: $4.([]*ast.IndexColName),
+                       OnDelete: onDeleteOpt,
+                       OnUpdate: onUpdateOpt,
+               }
+       }
 
 OnDeleteOpt:
        {

[morgo]

Thank you for reporting this issue!

For some added context, FOREIGN KEYS are currently only partially supported by TiDB. Full support is also not on the immediate roadmap.

Having said that, I think supporting ON UPDATE ddl statements in the parser is independently useful:

• For those using the parser independent of TiDB.
• For TiDB being able to accept more DDL statements.

Are you interested in contributing this as a pull request?

[EmmaLouise2018]

Sure. I can put up a PR.

[winkyao]

@EmmaLouise2018 Thank you. If you encounter any problem when you resolving this, let us know in here.

[EmmaLouise2018]

This is actually my first time working with Yacc. Following my train of thought from above, I included the additional rule in the diff. Running make test, generates a parser.go file, which when the tests are run they all pass. However, it also introduces, perhaps unsurprisingly reduce/reduce conflicts that I am not sure how to resolve.

I've opened up this PR and would appreciate any help/input you may have #286

[gleonid]

here is another PR, that does not have grammar conflicts
#331

[gleonid]

PR #331 has been merged.
issue can be closed