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
