Repository navigation
PostgreSQL: Add support for DO statement - #2618
devtechedge wants to merge 9 commits into
Conversation
|
Please add examples of this syntax to the fuzzer corpus |
LucaCappelletti94
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2618 +/- ##
==========================================
+ Coverage 81.24% 81.27% +0.02%
==========================================
Files 42 42
Lines 33901 33951 +50
Branches 33901 33951 +50
==========================================
+ Hits 27543 27592 +49
+ Misses 2792 2790 -2
- Partials 3566 3569 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bad1741 to
0198d07
Compare
|
@LucaCappelletti94 Good call, the fuzzer should know about this syntax too. I added five seeds under Together they cover a plain fmt, clippy and the full test suite pass with them added. Happy to add or reshape any of them if you had other cases in mind 🙂 |
| Do { | ||
| /// Optional procedural language name. | ||
| language: Option<Ident>, | ||
| /// The anonymous code block, a string literal. | ||
| code: ValueWithSpan, | ||
| }, |
There was a problem hiding this comment.
Refactor this to a named struct, as every new statement variant does. I will do another review round after this refactoring as I expect most of the code will change.
| Do { | |
| /// Optional procedural language name. | |
| language: Option<Ident>, | |
| /// The anonymous code block, a string literal. | |
| code: ValueWithSpan, | |
| }, | |
| Do(DoStatement), |
There was a problem hiding this comment.
Thanks @LucaCappelletti94!
I refactored it into a named DoStatement struct (Statement::Do(DoStatement)), with its own Display and From<DoStatement> for Statement, following the pattern of the other statement structs.
I also gave it a Spanned impl covering the language and code block, with a small span test.
Parsing behavior and existing tests are unchanged, and fmt, clippy and the full test suite pass.
Ready for another look whenever you have time.
f58b626 to
adc68d1
Compare
| match &self.peek_token_ref().token { | ||
| Token::SingleQuotedString(_) | ||
| | Token::DollarQuotedString(_) | ||
| | Token::EscapedStringLiteral(_) => {} |
There was a problem hiding this comment.
You should accept the Unicode escape string as a code block too. PostgreSQL 18 runs DO U&'BEGIN NULL; END', and the PostgreSQL dialect tokenizes it as Token::UnicodeStringLiteral, so this match rejects it with Expected: string literal, found: U&'BEGIN NULL; END'.
| | Token::EscapedStringLiteral(_) => {} | |
| | Token::EscapedStringLiteral(_) | |
| | Token::UnicodeStringLiteral(_) => {} |
| _ => unreachable!(), | ||
| } | ||
|
|
||
| dialects.verified_stmt("DO 'BEGIN NULL; END'"); |
There was a problem hiding this comment.
And its red test.
| dialects.verified_stmt("DO 'BEGIN NULL; END'"); | |
| dialects.verified_stmt("DO 'BEGIN NULL; END'"); | |
| dialects.verified_stmt("DO U&'BEGIN NULL; END'"); |
|
Thanks @LucaCappelletti94. I've applied both suggestions: parse_do now accepts Unicode escape strings (U&'...') as the code block, and I added DO U&'BEGIN NULL; END' to parse_do_statement. fmt and the full test suite pass locally. |
| pub fn parse_do(&mut self) -> Result<Statement, ParserError> { | ||
| let mut language = self.parse_do_language()?; | ||
| match &self.peek_token_ref().token { | ||
| Token::SingleQuotedString(_) | ||
| | Token::DollarQuotedString(_) | ||
| | Token::EscapedStringLiteral(_) | ||
| | Token::UnicodeStringLiteral(_) => {} | ||
| _ => return self.expected_ref("string literal", self.peek_token_ref()), | ||
| } | ||
| let code = self.parse_value()?; | ||
| if language.is_none() { | ||
| language = self.parse_do_language()?; | ||
| } | ||
| Ok(Statement::Do(DoStatement { language, code })) | ||
| } | ||
|
|
||
| fn parse_do_language(&mut self) -> Result<Option<Ident>, ParserError> { | ||
| if self.parse_keyword(Keyword::LANGUAGE) { | ||
| Ok(Some(self.parse_identifier()?)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
You should parse DO the way PostgreSQL's dostmt_opt_list does. That means any run of code blocks and LANGUAGE items, with the name either an identifier or any string literal, and the missing-code and duplicate checks left to the server. The repo's fuzz_postgres_accepts target panics on DO LANGUAGE plpgsql, DO $$a$$ $$b$$, a repeated LANGUAGE and DO LANGUAGE $$plpgsql$$ $$a$$, all of which PostgreSQL's grammar accepts, and fuzzing from this PR's seeds reaches one within seconds.
Please use the fuzzer locally before the next review round to catch these things.
| pub fn parse_do(&mut self) -> Result<Statement, ParserError> { | |
| let mut language = self.parse_do_language()?; | |
| match &self.peek_token_ref().token { | |
| Token::SingleQuotedString(_) | |
| | Token::DollarQuotedString(_) | |
| | Token::EscapedStringLiteral(_) | |
| | Token::UnicodeStringLiteral(_) => {} | |
| _ => return self.expected_ref("string literal", self.peek_token_ref()), | |
| } | |
| let code = self.parse_value()?; | |
| if language.is_none() { | |
| language = self.parse_do_language()?; | |
| } | |
| Ok(Statement::Do(DoStatement { language, code })) | |
| } | |
| fn parse_do_language(&mut self) -> Result<Option<Ident>, ParserError> { | |
| if self.parse_keyword(Keyword::LANGUAGE) { | |
| Ok(Some(self.parse_identifier()?)) | |
| } else { | |
| Ok(None) | |
| } | |
| } | |
| pub fn parse_do(&mut self) -> Result<Statement, ParserError> { | |
| let mut options = Vec::new(); | |
| loop { | |
| let language = self.parse_keyword(Keyword::LANGUAGE); | |
| let string_literal = matches!( | |
| self.peek_token_ref().token, | |
| Token::SingleQuotedString(_) | |
| | Token::DollarQuotedString(_) | |
| | Token::EscapedStringLiteral(_) | |
| | Token::UnicodeStringLiteral(_) | |
| ); | |
| let option = match (language, string_literal) { | |
| (true, true) => DoOption::LanguageLiteral(self.parse_value()?), | |
| (true, false) => DoOption::Language(self.parse_identifier()?), | |
| (false, true) => DoOption::Code(self.parse_value()?), | |
| (false, false) if options.is_empty() => { | |
| return self.expected_ref("string literal", self.peek_token_ref()); | |
| } | |
| (false, false) => break, | |
| }; | |
| options.push(option); | |
| } | |
| Ok(Statement::Do(DoStatement { options })) | |
| } |
| pub struct DoStatement { | ||
| /// Optional procedural language name. | ||
| pub language: Option<Ident>, | ||
| /// The anonymous code block, a string literal. | ||
| pub code: ValueWithSpan, | ||
| } | ||
|
|
||
| impl fmt::Display for DoStatement { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "DO ")?; | ||
| if let Some(language) = &self.language { | ||
| write!(f, "LANGUAGE {language} ")?; | ||
| } | ||
| write!(f, "{}", self.code) | ||
| } | ||
| } |
There was a problem hiding this comment.
You should store the items in source order, so every accepted form prints back as written.
| pub struct DoStatement { | |
| /// Optional procedural language name. | |
| pub language: Option<Ident>, | |
| /// The anonymous code block, a string literal. | |
| pub code: ValueWithSpan, | |
| } | |
| impl fmt::Display for DoStatement { | |
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |
| write!(f, "DO ")?; | |
| if let Some(language) = &self.language { | |
| write!(f, "LANGUAGE {language} ")?; | |
| } | |
| write!(f, "{}", self.code) | |
| } | |
| } | |
| pub struct DoStatement { | |
| /// The code blocks and `LANGUAGE` options, in source order. | |
| pub options: Vec<DoOption>, | |
| } | |
| impl fmt::Display for DoStatement { | |
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |
| write!(f, "DO {}", display_separated(&self.options, " ")) | |
| } | |
| } | |
| /// An item of a [`DoStatement`]. | |
| #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] | |
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | |
| #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] | |
| pub enum DoOption { | |
| /// `LANGUAGE lang_name` | |
| Language(Ident), | |
| /// `LANGUAGE` followed by the name as a string literal. | |
| LanguageLiteral(ValueWithSpan), | |
| /// The anonymous code block, a string literal. | |
| Code(ValueWithSpan), | |
| } | |
| impl fmt::Display for DoOption { | |
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |
| match self { | |
| DoOption::Language(language) => write!(f, "LANGUAGE {language}"), | |
| DoOption::LanguageLiteral(language) => write!(f, "LANGUAGE {language}"), | |
| DoOption::Code(code) => write!(f, "{code}"), | |
| } | |
| } | |
| } |
| Join, JoinConstraint, JoinOperator, JsonPath, JsonPathElem, LateralView, LimitClause, | ||
| MatchRecognizePattern, Measure, Merge, MergeAction, MergeClause, MergeInsertExpr, | ||
| MergeInsertKind, MergeUpdateExpr, MergeUpdateKind, NamedParenthesizedList, | ||
| CreateTableOptions, Cte, Delete, DoStatement, DoUpdate, ExceptSelectItem, |
There was a problem hiding this comment.
You should import the new DoOption.
| CreateTableOptions, Cte, Delete, DoStatement, DoUpdate, ExceptSelectItem, | |
| CreateTableOptions, Cte, Delete, DoOption, DoStatement, DoUpdate, ExceptSelectItem, |
| impl Spanned for DoStatement { | ||
| fn span(&self) -> Span { | ||
| let DoStatement { language, code } = self; | ||
| union_spans( | ||
| language | ||
| .iter() | ||
| .map(|l| l.span) | ||
| .chain(core::iter::once(code.span)), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
You should span the option list, each item by its name or literal.
| impl Spanned for DoStatement { | |
| fn span(&self) -> Span { | |
| let DoStatement { language, code } = self; | |
| union_spans( | |
| language | |
| .iter() | |
| .map(|l| l.span) | |
| .chain(core::iter::once(code.span)), | |
| ) | |
| } | |
| } | |
| impl Spanned for DoStatement { | |
| fn span(&self) -> Span { | |
| union_spans(self.options.iter().map(|option| option.span())) | |
| } | |
| } | |
| impl Spanned for DoOption { | |
| fn span(&self) -> Span { | |
| match self { | |
| DoOption::Language(language) => language.span, | |
| DoOption::LanguageLiteral(value) | DoOption::Code(value) => value.span, | |
| } | |
| } | |
| } |
| match dialects.verified_stmt("DO $$BEGIN RAISE NOTICE 'hi'; END$$") { | ||
| Statement::Do(DoStatement { language, code }) => { | ||
| assert_eq!(language, None); | ||
| assert_eq!( | ||
| code.value, | ||
| Value::DollarQuotedString(DollarQuotedString { | ||
| value: "BEGIN RAISE NOTICE 'hi'; END".to_string(), | ||
| tag: None, | ||
| }) | ||
| ); | ||
| } | ||
| _ => unreachable!(), | ||
| } | ||
|
|
||
| match dialects.verified_stmt("DO LANGUAGE plpgsql $body$BEGIN NULL; END$body$") { | ||
| Statement::Do(DoStatement { language, .. }) => { | ||
| assert_eq!(language, Some(Ident::new("plpgsql"))); | ||
| } | ||
| _ => unreachable!(), | ||
| } | ||
|
|
||
| dialects.verified_stmt("DO 'BEGIN NULL; END'"); | ||
| dialects.verified_stmt("DO U&'BEGIN NULL; END'"); | ||
| dialects.one_statement_parses_to( | ||
| "DO $$BEGIN NULL; END$$ LANGUAGE plpgsql", | ||
| "DO LANGUAGE plpgsql $$BEGIN NULL; END$$", | ||
| ); | ||
| assert_eq!( | ||
| dialects | ||
| .parse_sql_statements("DO LANGUAGE plpgsql $$BEGIN NULL; END$$ LANGUAGE sql") | ||
| .unwrap_err(), | ||
| ParserError::ParserError("Expected: end of statement, found: LANGUAGE".to_string()) | ||
| ); |
There was a problem hiding this comment.
More red tests.
| match dialects.verified_stmt("DO $$BEGIN RAISE NOTICE 'hi'; END$$") { | |
| Statement::Do(DoStatement { language, code }) => { | |
| assert_eq!(language, None); | |
| assert_eq!( | |
| code.value, | |
| Value::DollarQuotedString(DollarQuotedString { | |
| value: "BEGIN RAISE NOTICE 'hi'; END".to_string(), | |
| tag: None, | |
| }) | |
| ); | |
| } | |
| _ => unreachable!(), | |
| } | |
| match dialects.verified_stmt("DO LANGUAGE plpgsql $body$BEGIN NULL; END$body$") { | |
| Statement::Do(DoStatement { language, .. }) => { | |
| assert_eq!(language, Some(Ident::new("plpgsql"))); | |
| } | |
| _ => unreachable!(), | |
| } | |
| dialects.verified_stmt("DO 'BEGIN NULL; END'"); | |
| dialects.verified_stmt("DO U&'BEGIN NULL; END'"); | |
| dialects.one_statement_parses_to( | |
| "DO $$BEGIN NULL; END$$ LANGUAGE plpgsql", | |
| "DO LANGUAGE plpgsql $$BEGIN NULL; END$$", | |
| ); | |
| assert_eq!( | |
| dialects | |
| .parse_sql_statements("DO LANGUAGE plpgsql $$BEGIN NULL; END$$ LANGUAGE sql") | |
| .unwrap_err(), | |
| ParserError::ParserError("Expected: end of statement, found: LANGUAGE".to_string()) | |
| ); | |
| match dialects.verified_stmt("DO $$BEGIN RAISE NOTICE 'hi'; END$$") { | |
| Statement::Do(DoStatement { options }) => assert_eq!( | |
| options, | |
| vec![DoOption::Code( | |
| Value::DollarQuotedString(DollarQuotedString { | |
| value: "BEGIN RAISE NOTICE 'hi'; END".to_string(), | |
| tag: None, | |
| }) | |
| .with_empty_span() | |
| )] | |
| ), | |
| _ => unreachable!(), | |
| } | |
| match dialects.verified_stmt("DO LANGUAGE plpgsql $body$BEGIN NULL; END$body$") { | |
| Statement::Do(DoStatement { options }) => { | |
| assert_eq!(options[0], DoOption::Language(Ident::new("plpgsql"))); | |
| } | |
| _ => unreachable!(), | |
| } | |
| dialects.verified_stmt("DO 'BEGIN NULL; END'"); | |
| dialects.verified_stmt("DO U&'BEGIN NULL; END'"); | |
| dialects.verified_stmt("DO $$BEGIN NULL; END$$ LANGUAGE plpgsql"); | |
| dialects.verified_stmt("DO LANGUAGE 'plpgsql' $$BEGIN NULL; END$$"); | |
| dialects.verified_stmt("DO LANGUAGE $$plpgsql$$ $$BEGIN NULL; END$$"); | |
| dialects.verified_stmt("DO $$BEGIN NULL; END$$ LANGUAGE E'plpgsql'"); | |
| dialects.verified_stmt("DO LANGUAGE plpgsql"); | |
| dialects.verified_stmt("DO $$BEGIN NULL; END$$ $$BEGIN NULL; END$$"); | |
| dialects.verified_stmt("DO LANGUAGE plpgsql LANGUAGE sql $$BEGIN NULL; END$$"); |
Accept any run of code blocks and LANGUAGE items, with the language name as an identifier or any string literal, and keep them in source order so every accepted form round-trips. Missing-code and duplicate checks are left to the server, matching PostgreSQL's grammar.
|
Thanks, both reviews are addressed in 4e846e6.
I added all your test cases. I ran What it did turn up are older issues elsewhere:
Happy to look at those in separate PRs. |
4e846e6 to
3ce05be
Compare
# Conflicts: # tests/sqlparser_common.rs
LucaCappelletti94
left a comment
LucaCappelletti94
left a comment
There was a problem hiding this comment.
Also add this red test in the postgres test suite. Please do check whether I missed any other DO related syntaxes, I am not omniscient.
dialects.verified_stmt("DO LANGUAGE plpgsql LANGUAGE sql $$BEGIN NULL; END$$");
dialects.one_statement_parses_to(
"DO LANGUAGE N'BEGIN NULL; END'",
"DO LANGUAGE nchar 'BEGIN NULL; END'",
);| ); | ||
| let option = match (language, string_literal) { | ||
| (true, true) => DoOption::LanguageLiteral(self.parse_value()?), | ||
| (true, false) => DoOption::Language(self.parse_identifier()?), |
There was a problem hiding this comment.
You should read LANGUAGE N'...' the way PostgreSQL does, as the language nchar followed by a code block. PostgreSQL's scanner splits N'...' into the keyword NCHAR and a string, and NCHAR is a non-reserved word, so DO LANGUAGE N'BEGIN NULL; END' passes pg_query. fuzz_postgres_accepts panics on it with Expected: identifier, found: N'BEGIN NULL; END', reached from this PR's seeds in about two minutes.
| (true, false) => DoOption::Language(self.parse_identifier()?), | |
| (true, false) => match &self.peek_token_ref().token { | |
| // PostgreSQL lexes `N'...'` as the keyword `NCHAR` followed by a string | |
| Token::NationalStringLiteral(code) => { | |
| let code = Value::SingleQuotedString(code.clone()); | |
| let span = self.next_token().span; | |
| options.push(DoOption::Language(Ident::with_span(span, "nchar"))); | |
| DoOption::Code(code.with_span(span)) | |
| } | |
| _ => DoOption::Language(self.parse_identifier()?), | |
| }, |
|
Thank you for the detailed review and for the suggested fix, I really appreciate it. I will add the test in the Postgres suite and implement the I will also check for any other So I can plan my time well, could you please give me a rough idea of how many more rounds of changes you expect for this PR? I am very happy to keep iterating until you are satisfied with it, and a rough sense of the remaining scope will simply help me plan and make sure I cover everything carefully. |
Absolutely no idea, until we can't find any fault in it anymore I guess. |
3 participants
DOblocks likeDO $$BEGIN RAISE NOTICE 'hi'; END$$currently fail with "Expected: an SQL statement, found: DO".This adds
Statement::Do { language, code }for PostgreSQL'sDOstatement, gated behind a newDialect::supports_do_statement()that is enabled for PostgreSQL.The code block must be a string literal (single-quoted, dollar-quoted, or
E''), andLANGUAGE langis accepted either before or after it, as the PostgreSQL grammar allows.Display normalizes to the documented
DO [ LANGUAGE lang ] codeorder, and a repeatedLANGUAGEis rejected.Tests in
tests/sqlparser_common.rscover both orders, the round trip, the error cases, and dialects without support.The implementation and tests were drafted with an AI coding assistant (Grok 4.7 effort: xHigh), and I reviewed the diff, ran the test suite and the fuzzer, and stand by the change.
Fixes #2609