Skip to content

Conversation

stevenliebregt
Copy link

This changes the parsing of the CREATE SEQUENCE options to allow the individual options to appear in any order.

The reason I'm opening this pull request is that the Postgres pg_dump utility dumps CREATE SEQUENCE statements like this:

CREATE SEQUENCE name6
AS INTEGER
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1

Where the START WITH 1 appears before the INCREMENT BY 1, Postgres allows this but the parser did not.

There should not be any noticable performance impact I think. I did run the benchmarks and did not see any impact.

Comment on lines +313 to +320
let sql9 = "CREATE SEQUENCE name6
AS INTEGER
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1";
pg().one_statement_parses_to(sql9, "CREATE SEQUENCE name6 AS INTEGER START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE CACHE 1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let sql9 = "CREATE SEQUENCE name6
AS INTEGER
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1";
pg().one_statement_parses_to(sql9, "CREATE SEQUENCE name6 AS INTEGER START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE CACHE 1");
pg().verified_stmt("CREATE SEQUENCE name6 AS INTEGER START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE CACHE 1");

it looks like this should be equivalent?

Comment on lines +17031 to +17032
let mut should_continue_loop = true;
while should_continue_loop {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut should_continue_loop = true;
while should_continue_loop {
loop {

It looks like we can skip the bool var and instead use a combination of continue and break to manage the loop?

Err(ParserError::ParserError(_))
));

let sql9 = "CREATE SEQUENCE name6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case with duplicate options?

@stevenliebregt
Copy link
Author

@iffyio thanks for the review, I'll be unable to address the comments for the next 2 weeks, but after that I'll address them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants