[Review] Prevent the specification of conflicting transaction read/write options - Mailing list pgsql-hackers

<div class="WordSection1"><p class="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">ChetanSuttraway <chetan(dot)suttraway(at)enterprisedb(dot)com> wrote:</span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black"> </span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> This is regarding
theTODO item:</span><p class="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">>"Prevent the specification of conflicting transaction read/write</span><p class="MsoNormal"><span
lang="EN"style="font-size:13.0pt;font-family:"Courier New";color:black">> options"</span><p class="MsoNormal"><span
lang="EN"style="font-size:13.0pt;font-family:"Courier New";color:black">> </span><p class="MsoNormal"><span
lang="EN"style="font-size:13.0pt;font-family:"Courier New";color:black">> listed at:</span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> <a
href="http://wiki.postgresql.org/wiki/Todo">http://wiki.postgresql.org/wiki/Todo</a></span><pclass="MsoNormal"><span
lang="EN"style="font-size:13.0pt;font-family:"Courier New";color:black"> </span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Here'smy review of this patch.</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Basicstuff:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">------------</span><br/><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Patch applies OK </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Compiles cleanly with no warnings</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-changes in gram.y do not introduce any conflicts. </span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">- Regression tests pass.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Documentation changes not required as the scenario is
obvious.</span><br/><br /><b><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">What it
does:</span></b><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-----------------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Preventthe specification of conflicting transaction mode
options<br />in the commands such as <br />START/BEGIN TRANSACTION, SET TRANSACTION, SET SESSION CHARACTERISTICS AS
TRANSACTIONtransaction_mode. <br /><br />Conflicting transaction modes include <br />1. READ WRITE | READ ONLY <br
/><br/>2. ISOLATION LEVEL { SERIALIZABLE | REPEATABLE READ | READ COMMITTED | READ UNCOMMITTED } <br /><br />3.
DEFERRABLE| NOT DEFERABLE </span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Thechanges are done in gram.y file to add a new function
whichchecks for conflicting transaction modes.<br /></span><tt><span style="font-size:10.0pt">  </span></tt><br /><br
/><b><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">Testing:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">------------</span><br/><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">postgres=#BEGIN TRANSACTION read only read write;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">ERROR:  conflicting options</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">LINE1: BEGIN TRANSACTION read only read write;</span><br
/><br/><tt><span style="font-size:10.0pt">postgres=# SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ
COMMITTEDISOLATION LEVEL READ unCOMMITTED;</span></tt><br /><tt><span style="font-size:10.0pt">ERROR:  conflicting
options</span></tt><br/><tt><span style="font-size:10.0pt">LINE 1: ...ON CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL
READCOMMI...</span></tt><br /><br /><tt><span style="font-size:10.0pt">postgres=# start transaction deferrable not
deferrable;</span></tt><br/><tt><span style="font-size:10.0pt">ERROR:  conflicting options</span></tt><br /><tt><span
style="font-size:10.0pt">LINE1: start transaction deferrable not deferrable;</span></tt><br /><tt><span
style="font-size:10.0pt">                         ^</span></tt><br /><tt><span style="font-size:10.0pt">postgres=#
starttransaction deferrable read only not deferrable;</span></tt><br /><tt><span style="font-size:10.0pt">ERROR:
 conflictingoptions</span></tt><br /><tt><span style="font-size:10.0pt">LINE 1: start transaction deferrable read only
notdeferrable;</span></tt><br /><tt><span style="font-size:10.0pt">                          ^</span></tt><br
/><tt><spanstyle="font-size:10.0pt">postgres=# start transaction deferrable, read only not deferrable;</span></tt><br
/><tt><spanstyle="font-size:10.0pt">ERROR:  conflicting options</span></tt><br /><tt><span
style="font-size:10.0pt">LINE1: start transaction deferrable, read only not deferrable;</span></tt><br /><tt><span
style="font-size:10.0pt">                         ^</span></tt><br /><tt><span style="font-size:10.0pt">postgres=#
starttransaction deferrable, read only, not deferrable;</span></tt><br /><tt><span style="font-size:10.0pt">ERROR:
 conflictingoptions</span></tt><br /><tt><span style="font-size:10.0pt">LINE 1: start transaction deferrable, read
only,not deferrable;</span></tt><br /><br /><br /><tt><span style="font-size:10.0pt">postgres=# start transaction
deferrable,read only, ISOLATION LEVEL READ COMMITTED;</span></tt><br /><tt><span style="font-size:10.0pt">START
TRANSACTION</span></tt><br/><tt><span style="font-size:10.0pt">postgres=# commit;</span></tt><br /><tt><span
style="font-size:10.0pt">COMMIT</span></tt><br/><tt><span style="font-size:10.0pt">postgres=# start transaction
deferrable,read only, ISOLATION LEVEL READ UNCOMMITTED;</span></tt><br /><tt><span style="font-size:10.0pt">START
TRANSACTION</span></tt><br/><tt><span style="font-size:10.0pt">postgres=# commit;</span></tt><br /><tt><span
style="font-size:10.0pt">COMMIT</span></tt><br/><tt><span style="font-size:10.0pt">postgres=# start transaction
deferrable,read only, ISOLATION LEVEL READ UNCOMMITTED, read only;</span></tt><br /><tt><span
style="font-size:10.0pt">STARTTRANSACTION</span></tt><br /><tt><span style="font-size:10.0pt">                         
                                  ^</span></tt><br /><tt><span style="font-size:10.0pt">postgres=# SET SESSION
CHARACTERISTICSAS TRANSACTION ISOLATION LEVEL READ COMMITTED ISOLATION LEVEL READ COMMITTED;</span></tt><br /><tt><span
style="font-size:10.0pt">SET</span></tt><br/><tt><span style="font-size:10.0pt">                         
^</span></tt><br/><br /><br /><b><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Code Review
Comments</span></b><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------------------------------</span><br/><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">1.</span><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Complexity of function check_trans_mode is high (n^2) can be
changedto n</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   we shall not add the
duplicateelements in </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif""> 
 transaction_mode_list:</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">                   
   transaction_mode_item</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">                 
                     { $$ = list_make1($1); }</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       | transaction_mode_list ','
transaction_mode_item</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">                     
                 {</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       
                       check_trans_mode((List *)$1, (DefElem *)$3, yyscanner);</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                                               <span
style="color:red">$$= lappend($1, $3);</span></span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                                       }</span><br /><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        | transaction_mode_list
transaction_mode_item</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">                     
                 {</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       
                       check_trans_mode((List *)$1, (DefElem *)$2, yyscanner);</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                                               <span
style="color:red">$$= lappend($1, $2);</span></span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                                       }</span><br /><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">   i,e if "start transaction read only read only" is
giventhen add the mode "read only" to the list only for the first time.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  If so length of the valid list is limited to maximum 3
elements(modes)(1 ISOLATION LEVEL, 1 READ WRITE/READ ONLY, 1 DEFERRABLE).</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  This will reduce the search complexity of check_trans_mode
to3n = n.</span><br /><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">2.</span><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">during parse when you call check_trans_mode((List *)$1,
(DefElem*)$3, yyscanner); what is the need</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  of casting first and second parameter?</span><br /><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">3.</span><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">The comment describing function can be more
specific</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   patch - checks for conflicting
transactionmodes by looking up current element in the given list.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  suggested - checks for conflicting transaction modes by
lookingcurrent transaction mode element </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif""> 
            in the given transaction mode list.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif""> </span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">4.</span><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">names used for function parameters and variables can be more
meaningful.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   a. check_trans_mode(List
*list,DefElem *elem, core_yyscan_t yyscanner)</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  for first parameter instead of list, you can use modes, it
canbetter suggest the meaning of parameter.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  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.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  b. A_Const *elem_arg; can be changed input_mode_arg
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   c. DefElem *next; - mode_item</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">      A_Const *arg;  - mode_item_arg</span><br /><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">5. elem_arg->val.val.str, other places in code
accessthe string or integer by using strVal or intVal.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">6.Code which checks duplicate value should be as follows.
Pleaserefer function ExecSetVariableStmt, case VAR_SET_MULTI</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   if (strcmp(elem->defname,"transaction_isolation") ==
0)</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">     ...</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   else if (strcmp(elem->defname,"transaction_read_only")
==0)</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">      ...</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   else if (strcmp(elem->defname,"transaction_deferrable")
==0) </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">      ...</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   else</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">        elog(ERROR, "option \"%s\" not recognized",</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                               
 elem->defname)</span><br/><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">7. Cursor which
indicateswhere the error has occurred is pointing to first occurrence of the parameter rather than where exactly the
conflicthappened. In below example the cursor should point to read only.  </span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Iam not sure whether this is okay or not. Committer can give
suggestionsif it needs to be corrected</span><p class="MsoNormal"><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">postgres=#start transaction deferrable, read write, ISOLATION
LEVELREAD UNCOMMITTED, read only;</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">ERROR:
 conflictingoptions</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">LINE 1: start
transactiondeferrable, <span style="color:maroon">read write,</span> ISOLATION LEVEL RE...</span><p
class="MsoNormal"><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                                     
^</span><pclass="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Conclusion</span></b><p
class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------------------------------</span><p
class="MsoNormal"> <pclass="MsoNormal">The patch needs to updated. <p class="MsoNormal">1. Regression suite should be
updatedwith test cases.<p class="MsoNormal">2.  Code should be modified for Code Review Comments.<p
class="MsoNormal"> <pclass="MsoNormal">So I am marking this as Waiting on Author<p class="MsoNormal"> <p
class="MsoNormal">WithRegards,<p class="MsoNormal">Amit Kapila</div>