-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support SET TRANSACTION
statements.
#81
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,18 @@ export interface BeginStatement extends PGNode { | |
deferrable?: boolean; | ||
} | ||
|
||
export interface SetTransactionSnapshot extends PGNode { | ||
type: 'set transaction snapshot'; | ||
snapshotId: Name; | ||
} | ||
|
||
export interface SetTransactionMode extends PGNode { | ||
type: 'set transaction' | 'set session characteristics as transaction'; | ||
isolationLevel: 'serializable' | 'repeatable read' | 'read committed' | 'read uncommitted'; | ||
writeable?: 'read write' | 'read only'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this maybe be a boolean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, if you use In fact, given that "writeable" is the default behaviour, it would perhaps be more relevent to have a That would allow code like: if (!node.readonlyMode) {
// do something if writeable
} to behave as expected when the writeability is not explicitely provided |
||
deferrable?: boolean; | ||
} | ||
|
||
export interface DoStatement extends PGNode { | ||
type: 'do'; | ||
language?: Name; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,11 @@ simplestatements_tablespace -> kw_tablespace word {% x => track(x, { | |
}) %} | ||
|
||
|
||
simplestatements_set -> kw_set (simplestatements_set_simple | simplestatements_set_timezone) {% last %} | ||
simplestatements_set -> kw_set (simplestatements_set_simple | ||
| simplestatements_set_timezone | ||
| simplestatements_set_session | ||
| simplestatements_set_transaction_snapshot | ||
| simplestatements_set_transaction) {% last %} | ||
|
||
simplestatements_set_timezone -> kw_time kw_zone simplestatements_set_timezone_val {% x => track(x, { type: 'set timezone', to: x[2] }) %} | ||
|
||
|
@@ -65,6 +69,27 @@ simplestatements_set_val_raw | |
|
||
simplestatements_show -> kw_show ident {% x => track(x, { type: 'show', variable: asName(x[1]) }) %} | ||
|
||
# https://www.postgresql.org/docs/current/sql-set-transaction.html | ||
simplestatements_set_transaction_snapshot | ||
-> kw_transaction kw_snapshot ident {% x => track(x, { type: 'set transaction snapshot', snapshotId: asName(x[2]) }) %} | ||
|
||
simplestatements_set_session | ||
-> kw_session kw_characteristics %kw_as kw_transaction | ||
(simplestatements_begin_isol | simplestatements_begin_writ | simplestatements_begin_def):* {% | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
i.e. set session characteristics as transaction deferrable , isolation level read uncommitted, read write AND set session characteristics as transaction deferrable isolation level read uncommitted read write are both valid. For instance, something like this should fix both issues
(please add UTs to check that the syntax with commas is OK too) |
||
x => track(x, { | ||
type: 'set session characteristics as transaction', | ||
...x[4].reduce((a: any, b: any) => ({...unwrap(a), ...unwrap(b)}), {}), | ||
}) | ||
%} | ||
|
||
simplestatements_set_transaction | ||
-> kw_transaction (simplestatements_begin_isol | simplestatements_begin_writ | simplestatements_begin_def):* {% | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same remark |
||
x => track(x, { | ||
type: 'set transaction', | ||
...x[1].reduce((a: any, b: any) => ({...unwrap(a), ...unwrap(b)}), {}), | ||
}) | ||
%} | ||
|
||
|
||
# https://www.postgresql.org/docs/current/sql-createschema.html | ||
create_schema -> (%kw_create kw_schema) kw_ifnotexists:? ident {% x => track(x, { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,6 +247,26 @@ describe('Simple statements', () => { | |
}) | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I understand, in both If true, this version should be added to the tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup |
||
checkStatement(`set transaction SNAPSHOT mysnapshot`, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the snapshot ID is supposed to be a string (this statement is not parsable against a real db) set transaction SNAPSHOT 'mysnapshot' is OK |
||
type: 'set transaction snapshot', | ||
snapshotId: { name: 'mysnapshot' }, | ||
}); | ||
|
||
checkStatement(`set transaction isolation level repeatable read read only not deferrable`, { | ||
type: 'set transaction', | ||
isolationLevel: 'repeatable read', | ||
writeable: 'read only', | ||
deferrable: false, | ||
}); | ||
|
||
checkStatement(`set session characteristics as transaction isolation level read uncommitted deferrable read write`, { | ||
type: 'set session characteristics as transaction', | ||
isolationLevel: 'read uncommitted', | ||
writeable: 'read write', | ||
deferrable: true, | ||
}); | ||
|
||
|
||
checkStatement(`select * from (select a from mytable) myalias(col_renamed)`, { | ||
type: 'select', | ||
columns: [{ expr: { type: 'ref', name: '*' } }], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might make sense to add more granular types? (see suggestion by @oguimbal in #76)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it should: this typing suggests that
set session characteristics as transaction deferrable , isolation level read uncommitted , isolation level read committed , read write
is not a valid statement... but it is 🤷♂️ try it=> The fact that my typing suggestion involves an array of characteristics reflects that