Skip to content

Add support SET TRANSACTION statements. #81

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.

Sign up for GitHub

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

Open
jellelicht wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support SET TRANSACTION statements. #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/ast-mapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ export interface IAstPartialMapper {
createSequence?(seq: a.CreateSequenceStatement): a.Statement | nil
alterSequence?(seq: a.AlterSequenceStatement): a.Statement | nil
begin?(begin: a.BeginStatement): a.Statement | nil
setTransactionSnapshot?(snapshot: a.SetTransactionSnapshot): a.Statement | nil
setTransaction?(transaction: a.SetTransactionMode): a.Statement | nil
setSession?(session: a.SetTransactionMode): a.Statement | nil
}

export type IAstFullMapper = {
Expand Down Expand Up @@ -247,6 +250,12 @@ export class AstDefaultMapper implements IAstMapper {
return this.setGlobal(val);
case 'set timezone':
return this.setTimezone(val);
case 'set transaction snapshot':
return this.setTransactionSnapshot(val);
case 'set transaction':
return this.setTransaction(val);
case 'set session characteristics as transaction':
return this.setSession(val);
case 'create sequence':
return this.createSequence(val);
case 'alter sequence':
Expand Down Expand Up @@ -437,6 +446,17 @@ export class AstDefaultMapper implements IAstMapper {
return val;
}

setTransactionSnapshot?(snapshot: a.SetTransactionSnapshot): a.Statement | nil {
return snapshot;
}

setTransaction?(transaction: a.SetTransactionMode): a.Statement | nil {
return transaction;
}

setSession?(session: a.SetTransactionMode): a.Statement | nil {
return session;
}

update(val: a.UpdateStatement): a.Statement | nil {
if (!val) {
Expand Down
12 changes: 12 additions & 0 deletions src/syntax/ast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

@janpaepke janpaepke May 13, 2022

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)

Copy link
Owner

@oguimbal oguimbal May 16, 2022

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

type: 'set transaction' | 'set session characteristics as transaction';
isolationLevel: 'serializable' | 'repeatable read' | 'read committed' | 'read uncommitted';
writeable?: 'read write' | 'read only';
Copy link
Contributor

@janpaepke janpaepke May 13, 2022

Choose a reason for hiding this comment

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

Should this maybe be a boolean?

Copy link
Owner

@oguimbal oguimbal May 16, 2022

Choose a reason for hiding this comment

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

I agree, if you use deferable as boolean, it would make more sense to have a writeable as boolean as well.

In fact, given that "writeable" is the default behaviour, it would perhaps be more relevent to have a readonlyMode: boolean or something like that ?

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;
Expand Down
3 changes: 3 additions & 0 deletions src/syntax/base.ne
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ kw_filter -> %word {% notReservedKw('filter') %}
kw_commit -> %word {% notReservedKw('commit') %}
kw_tablespace -> %word {% notReservedKw('tablespace') %}
kw_transaction -> %word {% notReservedKw('transaction') %}
kw_snapshot -> %word {% notReservedKw('snapshot') %}
kw_session -> %word {% notReservedKw('session') %}
kw_characteristics -> %word {% notReservedKw('characteristics') %}
kw_work -> %word {% notReservedKw('work') %}
kw_read -> %word {% notReservedKw('read') %}
kw_write -> %word {% notReservedKw('write') %}
Expand Down
27 changes: 26 additions & 1 deletion src/syntax/simple-statements.ne
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] }) %}

Expand Down Expand Up @@ -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):* {%
Copy link
Owner

@oguimbal oguimbal May 16, 2022

Choose a reason for hiding this comment

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

  1. session characteristics may (optionally) be separated by commas.
  2. providing no transaction characteristic should not be valid (it is with this implementation)

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.
=> there shouldb be some kind of %comma:? somewhere :)

For instance, something like this should fix both issues

%kw_as kw_transaction sess_characteristics (%comma:? sess_characteristics):*

(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):* {%
Copy link
Owner

@oguimbal oguimbal May 16, 2022

Choose a reason for hiding this comment

The 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, {
Expand Down
20 changes: 20 additions & 0 deletions src/syntax/simple-statements.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,26 @@ describe('Simple statements', () => {
})


Copy link
Contributor

@janpaepke janpaepke May 13, 2022

Choose a reason for hiding this comment

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

From what I understand, in both SET TRANSACTION as well as SET SESSION CHARACTERISTICS [...] the transaction_modes can be separated with a comma. I think there was also one in your initial example? 🤔

If true, this version should be added to the tests.

Copy link
Owner

@oguimbal oguimbal May 16, 2022

Choose a reason for hiding this comment

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

yup

checkStatement(`set transaction SNAPSHOT mysnapshot`, {
Copy link
Owner

@oguimbal oguimbal May 16, 2022

Choose a reason for hiding this comment

The 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: '*' } }],
Expand Down
37 changes: 37 additions & 0 deletions src/to-sql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,43 @@ const visitor = astVisitor<IAstFullVisitor>(m => ({
}
},

setTransactionSnapshot: s => {
ret.push('SET TRANSACTION SNAPSHOT ');
ret.push(name(s.snapshotId));
},

setTransaction: tx => {
ret.push('SET TRANSACTION ');
if (tx.isolationLevel) {
ret.push('ISOLATION LEVEL ', tx.isolationLevel.toUpperCase(), ' ');
}
if (tx.writeable) {
ret.push(tx.writeable.toUpperCase(), ' ');
}
if (typeof tx.deferrable === 'boolean') {
if (!tx.deferrable) {
ret.push('NOT ');
}
ret.push('DEFERRABLE ');
}
},

setSession: s => {
ret.push('SET SESSION CHARACTERISTICS AS TRANSACTION ');
if (s.isolationLevel) {
ret.push('ISOLATION LEVEL ', s.isolationLevel.toUpperCase(), ' ');
}
if (s.writeable) {
ret.push(s.writeable.toUpperCase(), ' ');
}
if (typeof s.deferrable === 'boolean') {
if (!s.deferrable) {
ret.push('NOT ');
}
ret.push('DEFERRABLE ');
}
},

begin: beg => {
ret.push('BEGIN ');
if (beg.isolationLevel) {
Expand Down