Re: Nested xacts: looking for testers and review

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;
COMMIT
CREATE 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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073