[Review] Prevent the specification of conflicting transaction read/write options

Chetan Suttraway <chetan(dot)suttraway(at)enterprisedb(dot)com> wrote:

> This is regarding the TODO item:

> "Prevent the specification of conflicting transaction read/write

> options"

>

> listed at:

> http://wiki.postgresql.org/wiki/Todo

Here's my review of this patch.

Basic stuff:
------------

- Patch applies OK
- Compiles cleanly with no warnings
- changes in gram.y do not introduce any conflicts.
- Regression tests pass.
- Documentation changes not required as the scenario is obvious.

What it does:
-----------------------------
Prevent the specification of conflicting transaction mode options
in the commands such as
START/BEGIN TRANSACTION, SET TRANSACTION, SET SESSION CHARACTERISTICS AS
TRANSACTION transaction_mode.

Conflicting transaction modes include
1. READ WRITE | READ ONLY

2. ISOLATION LEVEL { SERIALIZABLE | REPEATABLE READ | READ COMMITTED | READ
UNCOMMITTED }

3. DEFERRABLE | NOT DEFERABLE

The changes are done in gram.y file to add a new function which checks for
conflicting transaction modes.

Testing:
------------

postgres=# BEGIN TRANSACTION read only read write;
ERROR: conflicting options
LINE 1: BEGIN TRANSACTION read only read write;

postgres=# SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ
COMMITTED ISOLATION LEVEL READ unCOMMITTED;
ERROR: conflicting options
LINE 1: ...ON CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMI...

postgres=# start transaction deferrable not deferrable;
ERROR: conflicting options
LINE 1: start transaction deferrable not deferrable;
^
postgres=# start transaction deferrable read only not deferrable;
ERROR: conflicting options
LINE 1: start transaction deferrable read only not deferrable;
^
postgres=# start transaction deferrable, read only not deferrable;
ERROR: conflicting options
LINE 1: start transaction deferrable, read only not deferrable;
^
postgres=# start transaction deferrable, read only, not deferrable;
ERROR: conflicting options
LINE 1: start transaction deferrable, read only, not deferrable;

postgres=# start transaction deferrable, read only, ISOLATION LEVEL READ
COMMITTED;
START TRANSACTION
postgres=# commit;
COMMIT
postgres=# start transaction deferrable, read only, ISOLATION LEVEL READ
UNCOMMITTED;
START TRANSACTION
postgres=# commit;
COMMIT
postgres=# start transaction deferrable, read only, ISOLATION LEVEL READ
UNCOMMITTED, read only;
START TRANSACTION
^
postgres=# SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ
COMMITTED ISOLATION LEVEL READ COMMITTED;
SET
^

Code Review Comments
----------------------------------------------

1. Complexity of function check_trans_mode is high (n^2) can be changed to n

we shall not add the duplicate elements in
transaction_mode_list:
transaction_mode_item
{ $$ = list_make1($1); }
| transaction_mode_list ',' transaction_mode_item
{
check_trans_mode((List *)$1,
(DefElem *)$3, yyscanner);
$$ = lappend($1, $3);
}

| transaction_mode_list transaction_mode_item
{
check_trans_mode((List *)$1,
(DefElem *)$2, yyscanner);
$$ = lappend($1, $2);
}

i,e if "start transaction read only read only" is given then add the mode
"read only" to the list only for the first time.
If so length of the valid list is limited to maximum 3 elements(modes) (1
ISOLATION LEVEL, 1 READ WRITE/READ ONLY, 1 DEFERRABLE).
This will reduce the search complexity of check_trans_mode to 3n = n.

2. during parse when you call check_trans_mode((List *)$1, (DefElem *)$3,
yyscanner); what is the need
of casting first and second parameter?

3. The comment describing function can be more specific
patch - checks for conflicting transaction modes by looking up current
element in the given list.
suggested - checks for conflicting transaction modes by looking current
transaction mode element
in the given transaction mode list.

4. names used for function parameters and variables can be more meaningful.
a. check_trans_mode(List *list, DefElem *elem, core_yyscan_t yyscanner)
for first parameter instead of list, you can use modes, it can better
suggest the meaning of parameter.
for second parameter instead of elem, you can use mode or input_mode; or
if something better is coming to your which can better suggest the meaning.

b. A_Const *elem_arg; can be changed input_mode_arg
c. DefElem *next; - mode_item
A_Const *arg; - mode_item_arg

5. elem_arg->val.val.str, other places in code access the string or integer
by using strVal or intVal.

6. Code which checks duplicate value should be as follows. Please refer
function ExecSetVariableStmt, case VAR_SET_MULTI
if (strcmp(elem->defname,"transaction_isolation") == 0)
...
else if (strcmp(elem->defname,"transaction_read_only") == 0)
...
else if (strcmp(elem->defname,"transaction_deferrable") == 0)
...
else
elog(ERROR, "option \"%s\" not recognized",
elem->defname)

7. Cursor which indicates where the error has occurred is pointing to first
occurrence of the parameter rather than where exactly the conflict happened.
In below example the cursor should point to read only.

I am not sure whether this is okay or not. Committer can give suggestions if
it needs to be corrected

postgres=# start transaction deferrable, read write, ISOLATION LEVEL READ
UNCOMMITTED, read only;
ERROR: conflicting options
LINE 1: start transaction deferrable, read write, ISOLATION LEVEL RE...

^

Conclusion

----------------------------------------------

The patch needs to updated.

1. Regression suite should be updated with test cases.

2. Code should be modified for Code Review Comments.

So I am marking this as Waiting on Author

With Regards,

Amit Kapila