Re: Nested xacts: looking for testers and review

Am I the only one who has a hard time understanding why COMMIT in the 
case of an error is allowed?  Since nothing is actually committed, but 
instead everything was actually rolled back.  Isn't it misleading to 
allow a commit under these circumstances?

Then to further extend the commit syntax with COMMIT WITHOUT ABORT makes 
even less since, IMHO.  If we are going to extend the syntax shouldn't 
we be extending ROLLBACK or END, something other than COMMIT so that we 
don't imply that anything was actually committed.

Perhaps I am being too literal here in reading the keyword COMMIT as 
meaning that something was actually committed, instead of COMMIT simply 
being end-of-transaction that may or may not have committed the changes 
in that transaction.  I have always looked at COMMIT and ROLLBACK as a 
symmetric pair of commands - ROLLBACK -> the changes in the transaction 
are not committed, COMMIT -> the changes in the transaction are 
committed.  That symmetry doesn't exist in reality since COMMIT only 
means that the changes might have been committed.

--Barry


Alvaro Herrera wrote:
> On Fri, May 28, 2004 at 04:05:40PM -0400, Bruce Momjian wrote:
> 
> Bruce,
> 
> 
>>One interesting idea would be for COMMIT to affect the outer
>>transaction, and END not affect the outer transaction.  Of course that
>>kills the logic that COMMIT and END are the same, but it is an
>>interesting idea, and doesn't affect backward compatibility because
>>END/COMMIT behave the same in non-nested transactions.
> 
> 
> I implemented this behavior by using parameters to COMMIT/END.  I didn't
> want to add new keywords to the grammar so I just picked up
> "COMMIT WITHOUT ABORT".  (Originally I had thought "COMMIT IGNORE
> ERRORS" but those would be two new keywords and I don't want to mess
> around with the grammar.  If there are different opinions, tweaking the
> grammar is easy).
> 
> So the behavior I originally implemented is still there:
> 
> alvherre=# begin;
> BEGIN
> alvherre=# begin;
> BEGIN
> alvherre=# select foo;
> ERROR:  no existe la columna "foo"
> alvherre=# commit;
> COMMIT
> alvherre=# select 1;
> ERROR:  transacción abortada, las consultas serán ignoradas hasta el fin de bloque de transacción
> alvherre=# commit;
> COMMIT
> 
> 
> However if one wants to use in script the behavior you propose, use
> the following:
> 
> alvherre=# begin;
> BEGIN
> alvherre=# begin;
> BEGIN
> alvherre=# select foo;
> ERROR:  no existe la columna "foo"
> alvherre=# commit without abort;
> COMMIT
> alvherre=# select 1;
>  ?column? 
> ----------
>         1
> (1 fila)
> 
> alvherre=# commit;
> COMMIT
> 
> 
> The patch is attached.  It applies only after the previous patch,
> obviously.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> diff -Ncr --exclude-from=diff-ignore 10bgwriter/src/backend/access/transam/xact.c
13commitOpt/src/backend/access/transam/xact.c
> *** 10bgwriter/src/backend/access/transam/xact.c    2004-06-08 17:34:49.000000000 -0400
> --- 13commitOpt/src/backend/access/transam/xact.c    2004-06-09 12:00:49.000000000 -0400
> ***************
> *** 2125,2131 ****
>    *    EndTransactionBlock
>    */
>   void
> ! EndTransactionBlock(void)
>   {
>       TransactionState s = CurrentTransactionState;
>   
> --- 2125,2131 ----
>    *    EndTransactionBlock
>    */
>   void
> ! EndTransactionBlock(bool ignore)
>   {
>       TransactionState s = CurrentTransactionState;
>   
> ***************
> *** 2163,2172 ****
>               /*
>                * here we are in an aborted subtransaction.  Signal
>                * CommitTransactionCommand() to clean up and return to the
> !              * parent transaction.
>                */
>           case TBLOCK_SUBABORT:
> !             s->blockState = TBLOCK_SUBENDABORT_ERROR;
>               break;
>   
>           case TBLOCK_STARTED:
> --- 2163,2177 ----
>               /*
>                * here we are in an aborted subtransaction.  Signal
>                * CommitTransactionCommand() to clean up and return to the
> !              * parent transaction.  If we are asked to ignore the errors
> !              * in the subtransaction, the parent can continue; else,
> !              * it has to be put in aborted state too.
>                */
>           case TBLOCK_SUBABORT:
> !             if (ignore)
> !                 s->blockState = TBLOCK_SUBENDABORT_OK;
> !             else
> !                 s->blockState = TBLOCK_SUBENDABORT_ERROR;
>               break;
>   
>           case TBLOCK_STARTED:
> diff -Ncr --exclude-from=diff-ignore 10bgwriter/src/backend/parser/gram.y 13commitOpt/src/backend/parser/gram.y
> *** 10bgwriter/src/backend/parser/gram.y    2004-06-03 20:46:48.000000000 -0400
> --- 13commitOpt/src/backend/parser/gram.y    2004-06-09 11:51:04.000000000 -0400
> ***************
> *** 225,232 ****
>                   target_list update_target_list insert_column_list
>                   insert_target_list def_list opt_indirection
>                   group_clause TriggerFuncArgs select_limit
> !                 opt_select_limit opclass_item_list transaction_mode_list
> !                 transaction_mode_list_or_empty
>                   TableFuncElementList
>                   prep_type_clause prep_type_list
>                   execute_param_clause
> --- 225,232 ----
>                   target_list update_target_list insert_column_list
>                   insert_target_list def_list opt_indirection
>                   group_clause TriggerFuncArgs select_limit
> !                 opt_select_limit opclass_item_list transaction_commit_opts
> !                 transaction_mode_list transaction_mode_list_or_empty
>                   TableFuncElementList
>                   prep_type_clause prep_type_list
>                   execute_param_clause
> ***************
> *** 3765,3782 ****
>                       n->options = $3;
>                       $$ = (Node *)n;
>                   }
> !             | COMMIT opt_transaction
>                   {
>                       TransactionStmt *n = makeNode(TransactionStmt);
>                       n->kind = TRANS_STMT_COMMIT;
> !                     n->options = NIL;
>                       $$ = (Node *)n;
>                   }
> !             | END_P opt_transaction
>                   {
>                       TransactionStmt *n = makeNode(TransactionStmt);
>                       n->kind = TRANS_STMT_COMMIT;
> !                     n->options = NIL;
>                       $$ = (Node *)n;
>                   }
>               | ROLLBACK opt_transaction
> --- 3765,3782 ----
>                       n->options = $3;
>                       $$ = (Node *)n;
>                   }
> !             | COMMIT opt_transaction transaction_commit_opts
>                   {
>                       TransactionStmt *n = makeNode(TransactionStmt);
>                       n->kind = TRANS_STMT_COMMIT;
> !                     n->options = $3;
>                       $$ = (Node *)n;
>                   }
> !             | END_P opt_transaction transaction_commit_opts
>                   {
>                       TransactionStmt *n = makeNode(TransactionStmt);
>                       n->kind = TRANS_STMT_COMMIT;
> !                     n->options = $3;
>                       $$ = (Node *)n;
>                   }
>               | ROLLBACK opt_transaction
> ***************
> *** 3827,3832 ****
> --- 3827,3841 ----
>               | READ WRITE { $$ = FALSE; }
>           ;
>   
> + transaction_commit_opts:
> +             WITHOUT ABORT_P
> +                 {
> +                     $$ = list_make1(makeDefElem("ignore_errors",
> +                                     makeBoolConst(true, false)));
> +                 }
> +             | /* EMPTY */
> +                 { $$ = NIL; }
> +         ;
>   
>   /*****************************************************************************
>    *
> diff -Ncr --exclude-from=diff-ignore 10bgwriter/src/backend/tcop/utility.c 13commitOpt/src/backend/tcop/utility.c
> *** 10bgwriter/src/backend/tcop/utility.c    2004-05-29 19:20:14.000000000 -0400
> --- 13commitOpt/src/backend/tcop/utility.c    2004-06-09 12:02:53.000000000 -0400
> ***************
> *** 351,357 ****
>                           break;
>   
>                       case TRANS_STMT_COMMIT:
> !                         EndTransactionBlock();
>                           break;
>   
>                       case TRANS_STMT_ROLLBACK:
> --- 351,374 ----
>                           break;
>   
>                       case TRANS_STMT_COMMIT:
> !                         {
> !                             bool ignore = false;
> ! 
> !                             if (stmt->options)
> !                             {
> !                                 ListCell    *head;
> ! 
> !                                 foreach(head, stmt->options)
> !                                 {
> !                                     DefElem        *item = (DefElem *) lfirst(head);
> ! 
> !                                     if (strcmp(item->defname, "ignore_errors") == 0)
> !                                         ignore = true;
> !                                 }
> !                             }
> ! 
> !                             EndTransactionBlock(ignore);
> !                         }
>                           break;
>   
>                       case TRANS_STMT_ROLLBACK:
> diff -Ncr --exclude-from=diff-ignore 10bgwriter/src/include/access/xact.h 13commitOpt/src/include/access/xact.h
> *** 10bgwriter/src/include/access/xact.h    2004-06-08 17:48:36.000000000 -0400
> --- 13commitOpt/src/include/access/xact.h    2004-06-09 12:00:19.000000000 -0400
> ***************
> *** 171,177 ****
>   extern void CommitTransactionCommand(void);
>   extern void AbortCurrentTransaction(void);
>   extern void BeginTransactionBlock(void);
> ! extern void EndTransactionBlock(void);
>   extern bool IsSubTransaction(void);
>   extern bool IsTransactionBlock(void);
>   extern bool IsTransactionOrTransactionBlock(void);
> --- 171,177 ----
>   extern void CommitTransactionCommand(void);
>   extern void AbortCurrentTransaction(void);
>   extern void BeginTransactionBlock(void);
> ! extern void EndTransactionBlock(bool ignore);
>   extern bool IsSubTransaction(void);
>   extern bool IsTransactionBlock(void);
>   extern bool IsTransactionOrTransactionBlock(void);
> 
> 
> ------------------------------------------------------------------------
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings