Add support `SET TRANSACTION` statements. #81

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

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


+    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support `SET TRANSACTION` statements. #81

Add support `SET TRANSACTION` statements. #81

jellelicht commented May 12, 2022

janpaepke left a comment

janpaepke May 13, 2022

oguimbal May 16, 2022

janpaepke May 13, 2022

oguimbal May 16, 2022

janpaepke May 13, 2022

oguimbal May 16, 2022

oguimbal left a comment

oguimbal May 16, 2022

oguimbal May 16, 2022

oguimbal May 16, 2022

oguimbal May 16, 2022

oguimbal May 16, 2022

oguimbal May 16, 2022

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

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


		checkStatement(`set transaction SNAPSHOT mysnapshot`, {

Add support SET TRANSACTION statements. #81

Are you sure you want to change the base?

Add support SET TRANSACTION statements. #81

Conversation

jellelicht commented May 12, 2022

janpaepke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguimbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support `SET TRANSACTION` statements. #81

Add support `SET TRANSACTION` statements. #81