Fix load dump with triggers#2151
Conversation
| } | ||
| } | ||
|
|
||
| fn tokenize_sql_keywords(text: &str) -> Vec<String> { |
There was a problem hiding this comment.
I think the risk here is that the tokenizer has some subtle bugs. Did you consider the sqlite-parser we already use elsewhere in the server?
There was a problem hiding this comment.
Firstly, I thought about the tradeoff of rewriting the load_dump function using existing parsers, like sqlite-parser, versus slightly modifying this function by introducing manual parsing with tokens but with potential pitfalls. I chose the latter. However, now I'm thinking that rewriting the initial function is not as bad as I imagined. I pushed the changes.
riazus
left a comment
riazus
left a comment
There was a problem hiding this comment.
Implemented the dump parser function using sqlite3_parser. The tradeoffs are mentioned in the comments. Open to discussion if needed.
There was a problem hiding this comment.
Note: given the new approach I think the previous message doesn't make sense anymore.
| snapshot_kind: text | ||
| --- | ||
| {"error":"The passed dump sql is invalid: msg: near \"COMMIT\": syntax error, sql: SELECT abs(-9223372036854775808) \n COMMIT;, offset: 43"} | ||
| {"error":"The passed dump sql is invalid: syntax error near 'COMMIT' at line 7, column 11"} |
There was a problem hiding this comment.
Note: following the previous snapshot message, I ought to introduce a lot of unpleasant changes in the code. I believe the new message is clear enough with reasonable code modifications.
| reader | ||
| .read_to_string(&mut dump_content) | ||
| .await | ||
| .map_err(|e| LoadDumpError::Internal(format!("Failed to read dump content: {}", e)))?; |
There was a problem hiding this comment.
I'm aware of the memory allocation here, but given that we need to provide the object from the heap to sqlite3_parser I decided to go with this option. Not sure if it is the best one though.
|
Thanks @riazus! |
2 participants
Resolves #2150.
Test cases implemented as well.