Re: Nested xacts: looking for testers and review - Mailing list pgsql-hackers

Well, the default behavior of COMMIT for an aborted subtransaction is
that it will abort the upper transaction too, so I think this is the
behavior you want.

We are considering allowing COMMIT IGNORE ABORT for scripts that want to
do a subtransaction, but don't care if it fails, and because it is a
script, they can't test the return value to send ROLLBACK:
BEGIN;BEGIN;DROP TABLE test;COMMITCREATE TABLE test(x int);COMMIT;

In this case you don't care if the DROP fails, but you do it all in a
subtransaction so the visibility happens all at once.

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

Barry Lind wrote:
> 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
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
>                http://www.postgresql.org/docs/faqs/FAQ.html
> 

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073