Transaction support

[SamuelMarks]

Basic transaction support

[benesch]

Thanks for the PR, @SamuelMarks. Transaction statements are a great addition!

I have one rather large comment about the design of the new AST node, though. From a syntactic perspective, BEGIN and COMMIT should be modeled as their own standalone statements. We don't want to be in the business of deciding exactly when and where a transaction starts and ends, as that's the job of a semantic analyzer or planner downstream. (For example, a ROLLBACK statement should also end a transaction.)

Would you be open to introducing separate SQLStartTransaction and SQLCommit variants, rather than the one SQLTransaction variant you've introduced? (Note that BEGIN is actually nonstandard syntax, albeit widely-supported syntax, and we typically try to use the standard SQL names where possible, which is why I've suggested naming the variant after START TRANSACTION instead.) We'll also want to support a SQLRollback variant, but it doesn't need to happen all at once.

Let me know if you'd like any help with the refactor!

[benesch]

Great, thanks for the refactor! I've pushed some additional changes to support the various options that START TRANSACTION and COMMIT accept, and added support for BEGIN, ROLLBACK, and SET TRANSACTION as well. Let me know if I've somehow made this harder for you to use downstream, but I think the patch is strictly more general than it was before.

@nickolay, can I get your eyes on this when you have a minute?

[coveralls]

Pull Request Test Coverage Report for Build 317

• 168 of 182 (92.31%) changed or added relevant lines in 3 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage remained the same at 91.557%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sqlast/mod.rs	39	40	97.5%
src/sqlparser.rs	61	63	96.83%
tests/sqlparser_common.rs	68	79	86.08%

Files with Coverage Reduction	New Missed Lines	%
src/sqlparser.rs	1	91.54%
tests/sqlparser_common.rs	2	91.57%

Totals
Change from base Build 315:	0.0%
Covered Lines:	3980
Relevant Lines:	4347

---

💛 - Coveralls

[nickolay]

@benesch I trust your review, especially since I'm not familiar with transaction control syntax.

I'd only like to remind that it might be helpful for the future work on dialects to explicitly mark the dialect-specific parts of the parser and the relevant tests.

[benesch]

@benesch I trust your review, especially since I'm not familiar with transaction control syntax.

Ok, great! Merging.

I'd only like to remind that it might be helpful for the future work on dialects to explicitly mark the dialect-specific parts of the parser and the relevant tests.

Sure thing. Perhaps surprisingly, the only dialect-specific bit here is that PostgreSQL allows omitting the commas between transaction modes. I'd previously only noted that in the test, but I've copied that comment to the parsing code too.