Add support `SET TRANSACTION` statements. by jellelicht · Pull Request #81 · oguimbal/pgsql-ast-parser

jellelicht · 2022-05-12T23:39:33Z

First attempt to fix #76; I went for more similarity to the existing BEGIN statement.
Please advise if I missed something, or if I've misunderstood the instructions.

janpaepke

Good stuff.

Have you seen @oguimbal's suggestion in #76 to use toStr(matchedStatement).toLowerCase()? Might make the extraction functions more readable?

janpaepke · 2022-05-13T08:22:06Z

src/syntax/ast.ts

+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';


Should this maybe be a boolean?

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

janpaepke · 2022-05-13T08:25:02Z

src/syntax/ast.ts

+    snapshotId: Name;
+}
+
+export interface SetTransactionMode extends PGNode {


might make sense to add more granular types? (see suggestion by @oguimbal in #76)

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

janpaepke · 2022-05-13T08:33:47Z

src/syntax/simple-statements.spec.ts

@@ -247,6 +247,26 @@ describe('Simple statements', () => {
    })




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.

oguimbal

Good job on this first PR :) Thanks !

Not bad at all.

There are several fixes to make this mergeable though

oguimbal · 2022-05-16T07:27:47Z

src/syntax/simple-statements.spec.ts

    })


+    checkStatement(`set transaction SNAPSHOT mysnapshot`, {


the snapshot ID is supposed to be a string (this statement is not parsable against a real db)

set transaction SNAPSHOT 'mysnapshot'

is OK

oguimbal · 2022-05-16T08:28:05Z

src/syntax/ast.ts

+    snapshotId: Name;
+}
+
+export interface SetTransactionMode extends PGNode {


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

oguimbal · 2022-05-16T08:30:39Z

src/syntax/ast.ts

+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';


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

oguimbal · 2022-05-16T08:34:12Z

src/syntax/simple-statements.ne

+
+simplestatements_set_session
+    -> kw_session kw_characteristics %kw_as kw_transaction
+                    (simplestatements_begin_isol | simplestatements_begin_writ | simplestatements_begin_def):* {%


session characteristics may (optionally) be separated by commas.

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)

oguimbal · 2022-05-16T08:34:31Z

src/syntax/simple-statements.ne

+                    %}
+
+simplestatements_set_transaction
+    -> kw_transaction (simplestatements_begin_isol | simplestatements_begin_writ | simplestatements_begin_def):* {%


same remark

oguimbal · 2022-05-16T08:35:18Z

src/syntax/simple-statements.spec.ts

@@ -247,6 +247,26 @@ describe('Simple statements', () => {
    })




jellelicht changed the title ~~Add support for initial support SET TRANSACTION statements.~~ Add support SET TRANSACTION statements. May 12, 2022

Add support for SET TRANSACTION statements.

961f222

jellelicht force-pushed the master branch from 0f9c197 to 961f222 Compare May 12, 2022 23:39

janpaepke reviewed May 13, 2022

View reviewed changes

oguimbal requested changes May 16, 2022

View reviewed changes

		@@ -247,6 +247,26 @@ describe('Simple statements', () => {
		})

Conversation

jellelicht commented May 12, 2022

Uh oh!

janpaepke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguimbal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants