Thread: Re: [GENERAL] Rollback on Error

Re: [GENERAL] Rollback on Error

From
"Michael Paesold"
Date:
Tom Lane wrote:

> "Michael Paesold" <mpaesold@gmx.at> writes:
> > On the other hand, the scenario of a psql option (read: I have
> > given up the idea of a backend implementation) to rollback only
> > last statement on error is quite different.
>
> Sure (and we already have one for autocommit).  But I thought you were
> asking about a backend implementation.

I have implemented what I have suggested for psql. I have attached a first
patch for review here, because I have a few questions. Also I want to make
sure the whole thing is reasonable.

I have named the option "IMPLICIT_SAVEPOINTS", because that's what it is. If
someone has a better name that would describe the purpose of the feature, I
am happy to change it.

The feature is activated, if
* \set IMPLICIT_SAVEPOINTS 'on'
* connection is in "idle in transaction" state
* psql session is interactive

The code executes an implicit "SAVEPOINT pg_internal_psql" in
common.c/SendQuery to which it will try to rollback to, if the executed
query fails.

Open questions:
* Should psql print a notice in the case of that rollback?
Something like "Rollback of last statement successful."?

* What is currently missing, is a detection of \i ... obviously this feature
should not be used for each query in \i. Perhaps only for the whole \i
command?
So what should I do to detect \i?
Add an extra argument to MainLoop, SendQuery and process_file()? (many
changes)
Add a global variable in common.c/h (e.g. bool
deactivate_implicit_savepoints) that can be used in process_file to
temporarily deactivate the code path?
(more local changes, but rather a hack imho)

Please have a look at the patch and comment.

Best Regards,
Michael Paesold

Attachment

Re: [GENERAL] Rollback on Error

From
Bruce Momjian
Date:
I assume this is to be saved for 8.1.

This has been saved for the 8.1 release:

    http:/momjian.postgresql.org/cgi-bin/pgpatches2

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

Michael Paesold wrote:
> Tom Lane wrote:
>
> > "Michael Paesold" <mpaesold@gmx.at> writes:
> > > On the other hand, the scenario of a psql option (read: I have
> > > given up the idea of a backend implementation) to rollback only
> > > last statement on error is quite different.
> >
> > Sure (and we already have one for autocommit).  But I thought you were
> > asking about a backend implementation.
>
> I have implemented what I have suggested for psql. I have attached a first
> patch for review here, because I have a few questions. Also I want to make
> sure the whole thing is reasonable.
>
> I have named the option "IMPLICIT_SAVEPOINTS", because that's what it is. If
> someone has a better name that would describe the purpose of the feature, I
> am happy to change it.
>
> The feature is activated, if
> * \set IMPLICIT_SAVEPOINTS 'on'
> * connection is in "idle in transaction" state
> * psql session is interactive
>
> The code executes an implicit "SAVEPOINT pg_internal_psql" in
> common.c/SendQuery to which it will try to rollback to, if the executed
> query fails.
>
> Open questions:
> * Should psql print a notice in the case of that rollback?
> Something like "Rollback of last statement successful."?
>
> * What is currently missing, is a detection of \i ... obviously this feature
> should not be used for each query in \i. Perhaps only for the whole \i
> command?
> So what should I do to detect \i?
> Add an extra argument to MainLoop, SendQuery and process_file()? (many
> changes)
> Add a global variable in common.c/h (e.g. bool
> deactivate_implicit_savepoints) that can be used in process_file to
> temporarily deactivate the code path?
> (more local changes, but rather a hack imho)
>
> Please have a look at the patch and comment.
>
> Best Regards,
> Michael Paesold

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match

--
  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, Pennsylvania 19073

Re: [GENERAL] Rollback on Error

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I assume this is to be saved for 8.1.

> This has been saved for the 8.1 release:
>     http:/momjian.postgresql.org/cgi-bin/pgpatches2

It is not remotely ready to apply yet, so please do not put it in the
queue.

            regards, tom lane

Re: [GENERAL] Rollback on Error

From
Bruce Momjian
Date:
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I assume this is to be saved for 8.1.
>
> > This has been saved for the 8.1 release:
> >     http:/momjian.postgresql.org/cgi-bin/pgpatches2
>
> It is not remotely ready to apply yet, so please do not put it in the
> queue.

That queue isn't for 8.1 ready-to-apply stuff. It just says "saved".

--
  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, Pennsylvania 19073

Re: [GENERAL] Rollback on Error

From
"Michael Paesold"
Date:
Bruce Momjian <pgman@candle.pha.pa.us> wrote:

>> Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> > I assume this is to be saved for 8.1.

I assumed that to, so I did not want to disturb any more now.

>> > This has been saved for the 8.1 release:
>> > http:/momjian.postgresql.org/cgi-bin/pgpatches2
>>

> Tom Lane wrote:
>> It is not remotely ready to apply yet, so please do not put it in the
>> queue.

I hope you will be willing to comment on the issues when times come. I am
not really satisfied myself, but without further discussion I did not want
to continue to work on it. Anyway, I understand this is not the right time
now (8.0 beta).

Best Regards,
Michael Paesold


Re: [GENERAL] Rollback on Error

From
Alvaro Herrera Munoz
Date:
On Fri, Oct 08, 2004 at 08:40:56PM +0200, Michael Paesold wrote:

> I hope you will be willing to comment on the issues when times come. I am
> not really satisfied myself, but without further discussion I did not want
> to continue to work on it. Anyway, I understand this is not the right time
> now (8.0 beta).

I think it would be wise to create a function to discover what savepoints
are available, and from that know when to release an automatically-
established savepoint.

Of course, the function would only work when the backend is not in abort
state, but I think that's a reasonable restriction.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"Ni aun el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)

Re: Rollback on Error

From
"Michael Paesold"
Date:
Alvaro Herrera Munoz wrote:

> I think it would be wise to create a function to discover what savepoints
> are available, and from that know when to release an automatically-
> established savepoint.
>
> Of course, the function would only work when the backend is not in abort
> state, but I think that's a reasonable restriction.

Ok, that would need a set returning function in the backend, right? Could 
you help me write it when time comes? (I don't feel backend hacking savvy.)

What about the resources consumed by savepoints
a) that can be freed by a RELEASE (trx state stack, what else?)
versus
b) that cannot be freed (xid locks, anything else?)

Is it worth the effort of extra work (not programmer's but runtime ;-)?

Bruce,
in Revision 1.1355 of the TODO you removed a line from the todo list that 
said:
-Use nested transactions to prevent syntax errors from aborting a 
transaction

I tought this is what I made a patch for, so shouldn't it be back for 8.1? 
Or at least something similar?

Best Regards,
Michael Paesold 



Re: Rollback on Error

From
Bruce Momjian
Date:
Michael Paesold wrote:
> Alvaro Herrera Munoz wrote:
> 
> > I think it would be wise to create a function to discover what savepoints
> > are available, and from that know when to release an automatically-
> > established savepoint.
> >
> > Of course, the function would only work when the backend is not in abort
> > state, but I think that's a reasonable restriction.
> 
> Ok, that would need a set returning function in the backend, right? Could 
> you help me write it when time comes? (I don't feel backend hacking savvy.)
> 
> What about the resources consumed by savepoints
> a) that can be freed by a RELEASE (trx state stack, what else?)
> versus
> b) that cannot be freed (xid locks, anything else?)
> 
> Is it worth the effort of extra work (not programmer's but runtime ;-)?
> 
> Bruce,
> in Revision 1.1355 of the TODO you removed a line from the todo list that 
> said:
> -Use nested transactions to prevent syntax errors from aborting a 
> transaction
> 
> I tought this is what I made a patch for, so shouldn't it be back for 8.1? 
> Or at least something similar?

The item was adjusted to be more specific:
* Add an option to automatically use savepoints for each statement in a  multi-statement transaction.  When enabled,
thiswould allow errors in multi-statement transactions  to be automatically ignored.
 

--  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
 


Re: [GENERAL] Rollback on Error

From
Bruce Momjian
Date:
This has already been implemented in CVS as a psql \set variable:

    ON_ERROR_ROLLBACK = 'interactive'

and will appear in 8.1.

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

Michael Paesold wrote:
> Tom Lane wrote:
>
> > "Michael Paesold" <mpaesold@gmx.at> writes:
> > > On the other hand, the scenario of a psql option (read: I have
> > > given up the idea of a backend implementation) to rollback only
> > > last statement on error is quite different.
> >
> > Sure (and we already have one for autocommit).  But I thought you were
> > asking about a backend implementation.
>
> I have implemented what I have suggested for psql. I have attached a first
> patch for review here, because I have a few questions. Also I want to make
> sure the whole thing is reasonable.
>
> I have named the option "IMPLICIT_SAVEPOINTS", because that's what it is. If
> someone has a better name that would describe the purpose of the feature, I
> am happy to change it.
>
> The feature is activated, if
> * \set IMPLICIT_SAVEPOINTS 'on'
> * connection is in "idle in transaction" state
> * psql session is interactive
>
> The code executes an implicit "SAVEPOINT pg_internal_psql" in
> common.c/SendQuery to which it will try to rollback to, if the executed
> query fails.
>
> Open questions:
> * Should psql print a notice in the case of that rollback?
> Something like "Rollback of last statement successful."?
>
> * What is currently missing, is a detection of \i ... obviously this feature
> should not be used for each query in \i. Perhaps only for the whole \i
> command?
> So what should I do to detect \i?
> Add an extra argument to MainLoop, SendQuery and process_file()? (many
> changes)
> Add a global variable in common.c/h (e.g. bool
> deactivate_implicit_savepoints) that can be used in process_file to
> temporarily deactivate the code path?
> (more local changes, but rather a hack imho)
>
> Please have a look at the patch and comment.
>
> Best Regards,
> Michael Paesold

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match

--
  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, Pennsylvania 19073