Re: Nested xacts: looking for testers and review

Hackers,

Ok, I've finally coded solutions to most problems regarding nested
transactions. This means:

- reversing for the lock manager, catcache, relcache, buffer manager,
asynchronous notifies, storage manager.

- transaction block state support, including appropiate XLog recording

- pg_subtrans subsystem (including changing state from SUBTRANS
COMMITTED to COMMITTED when appropiate). Also pg_clog XLog recovery
was handed to SLRU so pg_subtrans and pg_clog share a rmgr identity.

- visibility rules.

I'm missing one item: deferred triggers. The problem with this is that
the deftrig queue is not implemented using normal Lists, so there's no
efficient way to reassign to the parent when the subtransaction commits.
Also I'm not sure what should happen to the "immediate" pointer --- a
subtransaction should have it's own private copy, or it should inherit
the parent's? Please whoever implemented this speak up (Stephan
Szabo?), as I'm not sure of the semantics.

I have tested it and it passes all regression tests (including ones I
added), plus some more tests I threw at it mainly for concurrency.
Everything behaves as expected. At this time I'd like to have it
reviewed by the critic eye of the committers, and tested by whoever
would be using it.

I'm open for comments and suggestions and general input. Thank you.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
La web junta la gente porque no importa que clase de mutante sexual seas,
tienes millones de posibles parejas. Pon "buscar gente que tengan sexo con
ciervos incendiánse", y el computador dirá "especifique el tipo de ciervo"
(Jason Alexander)

On Wed, 26 May 2004, Alvaro Herrera wrote:

> I'm missing one item: deferred triggers. The problem with this is that
> the deftrig queue is not implemented using normal Lists, so there's no
> efficient way to reassign to the parent when the subtransaction commits.
> Also I'm not sure what should happen to the "immediate" pointer --- a
> subtransaction should have it's own private copy, or it should inherit
> the parent's? Please whoever implemented this speak up (Stephan
> Szabo?), as I'm not sure of the semantics.

[I haven't had a chance to look at the patch, because I'm at work, so you
may already have throught about/coded for some of the things below,
but...]

The immediate pointer basically points at the end of the queue from the
last scanning of the trigger queue (since any "immediate" triggers from
before that would have been run at that time there's no reason to scan
from the beginning).

As a related side question I just thought of (and wish I had earlier),
what's the correct behavior of SET CONSTRAINTS when setting a constraint
to immediate mode in the subtransaction case? In the single transaction
case, we change whether we consider the constraint deferred (or set the
"all"), reset the immediate pointer to the beginning of the queue, and
allow the end of statement scan to do what should hopefully be the right
thing. If one sets a constraint to immediate in a subtransaction,
does/should it cause the immediate check of pending events from its
parent? And does that revert when the transaction ends?

Before thinking about that, I figured that each subtransaction would have
a private trigger queue and then that the immediate pointer would be
private in each queue (starting as NULL when the subtransaction started)
and that if items were copied/reassigned at subtransaction end, the parent
transaction's immediate would move to just after that point after the
copy. Now, I'm not entirely sure.

On Wed, May 26, 2004 at 04:35:52PM -0700, Stephan Szabo wrote:

> On Wed, 26 May 2004, Alvaro Herrera wrote:
>
> > I'm missing one item: deferred triggers. The problem with this is that
> > the deftrig queue is not implemented using normal Lists, so there's no
> > efficient way to reassign to the parent when the subtransaction commits.
> > Also I'm not sure what should happen to the "immediate" pointer --- a
> > subtransaction should have it's own private copy, or it should inherit
> > the parent's? Please whoever implemented this speak up (Stephan
> > Szabo?), as I'm not sure of the semantics.
>
> The immediate pointer basically points at the end of the queue from the
> last scanning of the trigger queue (since any "immediate" triggers from
> before that would have been run at that time there's no reason to scan
> from the beginning).

Hmm. You assume correctly that a subtransaction has (or will have) a
private queue. But we do not consider a subtransaction to be somewhat a
separate entity -- the principle is that the transaction has to feel
just like the BEGIN wasn't there. So

BEGIN;
UPDATE foo ...
UPDATE bar ...
COMMIT

has to be exactly the same as

BEGIN;
BEGIN;
UPDATE foo ...
COMMIT;
BEGIN;
UPDATE bar ...
COMMIT;
COMMIT;

Now, with that in mind: is it correct that the "immediate" pointer
points to the beginning of the subtransaction's private deferred trigger
queue, at subtransaction's start?

Now, at subtransaction end, lets suppose I concatenate the list the
original transaction had with the subtransaction's private list. What
should the immediate pointer be?

When is the immediate pointer advanced? I know it's "during scanning of
the list", but when is this? At the end of each query?

> If one sets a constraint to immediate in a subtransaction, does/should
> it cause the immediate check of pending events from its parent? And
> does that revert when the transaction ends?

Yes, I think it should fire all events, including the parent's. Good
point; it means there has to be a way of getting the whole list, from
the topmost transaction, in order :-(

I'm not sure what you mean by reverting though.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
La web junta la gente porque no importa que clase de mutante sexual seas,
tienes millones de posibles parejas. Pon "buscar gente que tengan sexo con
ciervos incendiánse", y el computador dirá "especifique el tipo de ciervo"
(Jason Alexander)

On Thu, 27 May 2004, Alvaro Herrera wrote:

> On Wed, May 26, 2004 at 04:35:52PM -0700, Stephan Szabo wrote:
>
> > On Wed, 26 May 2004, Alvaro Herrera wrote:
> >
> > > I'm missing one item: deferred triggers. The problem with this is that
> > > the deftrig queue is not implemented using normal Lists, so there's no
> > > efficient way to reassign to the parent when the subtransaction commits.
> > > Also I'm not sure what should happen to the "immediate" pointer --- a
> > > subtransaction should have it's own private copy, or it should inherit
> > > the parent's? Please whoever implemented this speak up (Stephan
> > > Szabo?), as I'm not sure of the semantics.
> >
> > The immediate pointer basically points at the end of the queue from the
> > last scanning of the trigger queue (since any "immediate" triggers from
> > before that would have been run at that time there's no reason to scan
> > from the beginning).
>
> Hmm. You assume correctly that a subtransaction has (or will have) a
> private queue. But we do not consider a subtransaction to be somewhat a
> separate entity -- the principle is that the transaction has to feel
> just like the BEGIN wasn't there. So
>
> BEGIN;
> UPDATE foo ...
> UPDATE bar ...
> COMMIT
>
> has to be exactly the same as
>
> BEGIN;
> BEGIN;
> UPDATE foo ...
> COMMIT;
> BEGIN;
> UPDATE bar ...
> COMMIT;
> COMMIT;
>
> Now, with that in mind: is it correct that the "immediate" pointer
> points to the beginning of the subtransaction's private deferred trigger
> queue, at subtransaction's start?

AFAIR you can set it to NULL because that means scan the entire list.

> Now, at subtransaction end, lets suppose I concatenate the list the
> original transaction had with the subtransaction's private list. What
> should the immediate pointer be?

I'd say pointing at the last item that was added to the queue.

> When is the immediate pointer advanced? I know it's "during scanning of
> the list", but when is this? At the end of each query?

At the point after triggers fire after a statement. It's the actual
scanning of the trigger queue to see what to run that changes it unless
I'm misremembering.

> > If one sets a constraint to immediate in a subtransaction, does/should
> > it cause the immediate check of pending events from its parent? And
> > does that revert when the transaction ends?
>
> Yes, I think it should fire all events, including the parent's. Good
> point; it means there has to be a way of getting the whole list, from
> the topmost transaction, in order :-(

Yeah... Right now we don't need to do something special because resetting
the immediate pointer basically does what we want (re-scan the entire set
looking for non-run things that are now immediate).

> I'm not sure what you mean by reverting though.

The state about whether a trigger is actually deferred or immediate. I
believe it basically works as:
begin;
set constraints all immediate;
-- here any deferrable constraints are treated as immediate
end;
begin;
-- here any deferrable constraints are in their default state
end;

So, if you have
begin;
-- 1
begin;
set constraints all immediate;
end;
-- 2
end;

Do 1 and 2 see the same constraint checking mode or is 2 at immediate?

I have tested the patches with May 28 16:20 JST snapshot. Here is my
first impression:

1) errors, rules regression tests are failed (I'm not sure this is due
to your patches)

2) certain behavior was different from what I expected (please correct me
if my expectation is wrong).

test=# begin;
BEGIN
test=# insert into t1 values(1);
INSERT 17216 1
test=# begin;
BEGIN
test=# aaa;
ERROR: syntax error at or near "aaa" at character 1
ERROR: syntax error at or near "aaa" at character 1
LINE 1: aaa;
^
test=# end;
COMMIT
test=# select * from t1; <-- I thought this should work since subtransaction was closed
ERROR: current transaction is aborted, commands ignored until end of transaction block
ERROR: current transaction is aborted, commands ignored until end of transaction block
test=# end;
COMMIT
test=# select * from t1;
i
---
(0 rows)

3) no docs found in the patches.

> Hackers,
>
> Ok, I've finally coded solutions to most problems regarding nested
> transactions. This means:
>
> - reversing for the lock manager, catcache, relcache, buffer manager,
> asynchronous notifies, storage manager.
>
> - transaction block state support, including appropiate XLog recording
>
> - pg_subtrans subsystem (including changing state from SUBTRANS
> COMMITTED to COMMITTED when appropiate). Also pg_clog XLog recovery
> was handed to SLRU so pg_subtrans and pg_clog share a rmgr identity.
>
> - visibility rules.
>
> I'm missing one item: deferred triggers. The problem with this is that
> the deftrig queue is not implemented using normal Lists, so there's no
> efficient way to reassign to the parent when the subtransaction commits.
> Also I'm not sure what should happen to the "immediate" pointer --- a
> subtransaction should have it's own private copy, or it should inherit
> the parent's? Please whoever implemented this speak up (Stephan
> Szabo?), as I'm not sure of the semantics.
>
>
> I have tested it and it passes all regression tests (including ones I
> added), plus some more tests I threw at it mainly for concurrency.
> Everything behaves as expected. At this time I'd like to have it
> reviewed by the critic eye of the committers, and tested by whoever
> would be using it.
>
> I'm open for comments and suggestions and general input. Thank you.
>
> --
> Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
> La web junta la gente porque no importa que clase de mutante sexual seas,
> tienes millones de posibles parejas. Pon "buscar gente que tengan sexo con
> ciervos incendiánse", y el computador dirá "especifique el tipo de ciervo"
> (Jason Alexander)
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>

On Fri, May 28, 2004 at 04:51:07PM +0900, Tatsuo Ishii wrote:

> 2) certain behavior was different from what I expected (please correct me
> if my expectation is wrong).

Yes, the expected behavior is different: if an aborted subtransaction is
closed with a COMMIT or END command, the parent transaction is aborted
too. This is to inhibit an application which blindly expects the
subtransaction to succeed to reach an invalid state. If you want to
return to non-aborted state, end the subtransaction with ROLLBACK
instead.

But Bob Henkel and you are right: there needo to be documentation.
Initially I figured I could do that later because there is no new
syntax, but it is obviously needed to explain all sorts of assumptions
and behavior like this.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Limítate a mirar... y algun día veras"

> On Fri, May 28, 2004 at 04:51:07PM +0900, Tatsuo Ishii wrote:
>
> > 2) certain behavior was different from what I expected (please correct me
> > if my expectation is wrong).
>
> Yes, the expected behavior is different: if an aborted subtransaction is
> closed with a COMMIT or END command, the parent transaction is aborted
> too. This is to inhibit an application which blindly expects the
> subtransaction to succeed to reach an invalid state. If you want to
> return to non-aborted state, end the subtransaction with ROLLBACK
> instead.

Oh, I got it. Thanks.

> But Bob Henkel and you are right: there needo to be documentation.
> Initially I figured I could do that later because there is no new
> syntax, but it is obviously needed to explain all sorts of assumptions
> and behavior like this.
--
Tatsuo Ishii

Can I ask you one more question?

Is there any limit for nesting leveles of subtransactions?
--
Tatsuo Ishii

> On Fri, May 28, 2004 at 04:51:07PM +0900, Tatsuo Ishii wrote:
>
> > 2) certain behavior was different from what I expected (please correct me
> > if my expectation is wrong).
>
> Yes, the expected behavior is different: if an aborted subtransaction is
> closed with a COMMIT or END command, the parent transaction is aborted
> too. This is to inhibit an application which blindly expects the
> subtransaction to succeed to reach an invalid state. If you want to
> return to non-aborted state, end the subtransaction with ROLLBACK
> instead.
>
> But Bob Henkel and you are right: there needo to be documentation.
> Initially I figured I could do that later because there is no new
> syntax, but it is obviously needed to explain all sorts of assumptions
> and behavior like this.
>
> --
> Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
> "Limítate a mirar... y algun día veras"
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>

On Sat, May 29, 2004 at 12:27:39AM +0900, Tatsuo Ishii wrote:
> Can I ask you one more question?
>
> Is there any limit for nesting leveles of subtransactions?

In theory 2^16 I think, but I haven't tested it. It tried to 30 or so
only. Not sure if it's practical.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Escucha y olvidarás; ve y recordarás; haz y entenderás" (Confucio)

Alvaro Herrera wrote:
> On Fri, May 28, 2004 at 04:51:07PM +0900, Tatsuo Ishii wrote:
>
> > 2) certain behavior was different from what I expected (please correct me
> > if my expectation is wrong).
>
> Yes, the expected behavior is different: if an aborted subtransaction is
> closed with a COMMIT or END command, the parent transaction is aborted
> too. This is to inhibit an application which blindly expects the
> subtransaction to succeed to reach an invalid state. If you want to
> return to non-aborted state, end the subtransaction with ROLLBACK
> instead.

I am interested to know if people agree with this behavior, reproduced below:

test=# begin;
BEGIN
test=# insert into t1 values(1);
INSERT 17216 1
test=# begin;
BEGIN
test=# aaa;
ERROR: syntax error at or near "aaa" at character 1
ERROR: syntax error at or near "aaa" at character 1
LINE 1: aaa;
^
test=# end;
COMMIT
test=# select * from t1; <-- I thought this should work since
subtransaction was closed
ERROR: current transaction is aborted, commands ignored until end of
transaction block
ERROR: current transaction is aborted, commands ignored until end of
transaction block
test=# end;
COMMIT

The problem I see with the behavior shown is that there is no way to use
subtransactions in scripts, where the queries can't be changed.
Consider this:

BEGIN;
DO SOME WORK...
BEGIN;
INSERT ...
COMMIT;
BEGIN;
INSERT ...
COMMIT;
BEGIN;
INSERT ...
COMMIT;
BEGIN;
INSERT ...
COMMIT;
DO MORE WORK...
COMMIT;

In this case, I want to try all of the inserts, but any of them can
fail, then I want the bottom part done.

I guess my big question is that if they issue a COMMIT for a
subtransaction that failed, do we assume they made a mistake and fail
the outer transaction, or do we just accept it and not affect the outer
transaction.

In my logic, the subtransaction COMMIT is part of the subtransaction and
should not affect the outer transaction's state.

Unfortunately, we don't have any similar behavior in our 7.4 code
because whether you issue COMMIT or ABORT, it does not affect the outer
session.

Do any other databases have nested transactions, and how to they handle
it?

I think we should issue a warning but not affect the outer transaction.

> But Bob Henkel and you are right: there needs to be documentation.
> Initially I figured I could do that later because there is no new
> syntax, but it is obviously needed to explain all sorts of assumptions
> and behavior like this.

Right now I think just posting examples will work fine. I think the
above case shows we are not ready for documentation yet. What I would
like is for folks to focus on testing so we can find any open issues
like this one and address them.

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

On Fri, May 28, 2004 at 01:43:16PM -0400, Bruce Momjian wrote:

> In this case, I want to try all of the inserts, but any of them can
> fail, then I want the bottom part done.

I wonder where everyone eas when I asked this question a lot of time
ago. I said I thought the behavior should be like I described, and no
one objected.

Personally I think it would be a mistake to allow the COMMIT for the
subtransaction to ignore the fact that the subxact was aborted. However
I realize what you are proposing, and maybe this can be implemented
using a parameter to COMMIT (indicating to not propagate the error if
it's in aborted state, but commit normally otherwise).

However if everyone disagrees, I can take that part out, and the code
would be simpler. IMHO however, it would be less reliable.

> In my logic, the subtransaction COMMIT is part of the subtransaction and
> should not affect the outer transaction's state.

In some cases yes, but not all. In others, the outer transaction could
trust that the inner one worked; to make the example you posted work,
I'd use a program rather than a script, and check the return values (or
the transaction state). If the subxact is in aborted state, issue
ROLLBACK and try again; if not, commit.

> Unfortunately, we don't have any similar behavior in our 7.4 code
> because whether you issue COMMIT or ABORT, it does not affect the outer
> session.

Of course. This is new functionality.

> Right now I think just posting examples will work fine. I think the
> above case shows we are not ready for documentation yet. What I would
> like is for folks to focus on testing so we can find any open issues
> like this one and address them.

Ok.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
That's because in Europe they call me by name, and in the US by value!"

Alvaro Herrera wrote:
> On Fri, May 28, 2004 at 01:43:16PM -0400, Bruce Momjian wrote:
>
> > In this case, I want to try all of the inserts, but any of them can
> > fail, then I want the bottom part done.
>
> I wonder where everyone eas when I asked this question a lot of time
> ago. I said I thought the behavior should be like I described, and no
> one objected.

Sorry, I didn't understand the question at the time, or wasn't paying
attention.

> Personally I think it would be a mistake to allow the COMMIT for the
> subtransaction to ignore the fact that the subxact was aborted. However
> I realize what you are proposing, and maybe this can be implemented
> using a parameter to COMMIT (indicating to not propagate the error if
> it's in aborted state, but commit normally otherwise).
>
> However if everyone disagrees, I can take that part out, and the code
> would be simpler. IMHO however, it would be less reliable.

Imagine this case used in a script:

BEGIN;
DROP TABLE test;
CREATE TABLE test(x int);
COMMIT;

This will not work because the drop might fail. However you could use this:

BEGIN;
BEGIN;
DROP TABLE test;
COMMIT;
CREATE TABLE test(x int);
COMMIT;

It is done in a transaction so the table replace is an atomic operation.

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.

If this is the type of issue we are dealing with for the patch, I feel
very good. Good job Alvaro.

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

On Fri, May 28, 2004 at 04:05:40PM -0400, Bruce Momjian wrote:

Hm, you are right that there needs to be a more automatic way of doing
this.

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

How about "COMMIT SUB" and "END SUB"? I don't feel it's good to give
different meaning to COMMIT versus END, but this is only a gut kind of
thing and I could be convinced otherwise. It is even easier to
differentiate COMMIT/END than adding a parameter to them.

I mean, COMMIT SUB would not affect the state of the outer transaction,
while COMMIT would.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle." (Larry Wall, Apocalypse 6)

On Wed, 26 May 2004, Alvaro Herrera wrote:

> I have tested it and it passes all regression tests (including ones I
> added), plus some more tests I threw at it mainly for concurrency.
> Everything behaves as expected. At this time I'd like to have it
> reviewed by the critic eye of the committers, and tested by whoever
> would be using it.
>
> I'm open for comments and suggestions and general input. Thank you.

I unfortunately didn't really follow the discussions in the past (sorry :(
), but are the transaction state modifying statements done in a
subtransaction supposed to live beyond subtransaction rollback?

For example,
sszabo=# begin;
BEGIN
sszabo=# begin;
BEGIN
sszabo=# set transaction read only;
SET
sszabo=# select * from a;
a
---
6
(1 row)

sszabo=# rollback;
ROLLBACK
sszabo=# update a set a=6;
ERROR: transaction is read-only

On Fri, May 28, 2004 at 05:43:41PM -0700, Stephan Szabo wrote:

> On Wed, 26 May 2004, Alvaro Herrera wrote:
>
> > I have tested it and it passes all regression tests (including ones I
> > added), plus some more tests I threw at it mainly for concurrency.
> > Everything behaves as expected. At this time I'd like to have it
> > reviewed by the critic eye of the committers, and tested by whoever
> > would be using it.
>
> I unfortunately didn't really follow the discussions in the past (sorry :(
> ), but are the transaction state modifying statements done in a
> subtransaction supposed to live beyond subtransaction rollback?

Hmm, I suppose not.

I think this applies to all GUC variables, but I wonder if we want to
save the value of each one at subtransaction start and recover it at
abort? Things could easily get huge. Maybe only saving the ones that
are different from the default value, and from the last saved value.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)

Alvaro Herrera wrote:
> On Fri, May 28, 2004 at 05:43:41PM -0700, Stephan Szabo wrote:
>
> > On Wed, 26 May 2004, Alvaro Herrera wrote:
> >
> > > I have tested it and it passes all regression tests (including ones I
> > > added), plus some more tests I threw at it mainly for concurrency.
> > > Everything behaves as expected. At this time I'd like to have it
> > > reviewed by the critic eye of the committers, and tested by whoever
> > > would be using it.
> >
> > I unfortunately didn't really follow the discussions in the past (sorry :(
> > ), but are the transaction state modifying statements done in a
> > subtransaction supposed to live beyond subtransaction rollback?
>
> Hmm, I suppose not.
>
> I think this applies to all GUC variables, but I wonder if we want to
> save the value of each one at subtransaction start and recover it at
> abort? Things could easily get huge. Maybe only saving the ones that
> are different from the default value, and from the last saved value.

We have an on-commit field in the guc structures to handle
commit/rollback settings. Do we need to extend that to subtransactions?

I don't think you can save off only the defaults in an efficient manner.

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

On Fri, May 28, 2004 at 11:11:27PM -0400, Bruce Momjian wrote:
> Alvaro Herrera wrote:

> > I think this applies to all GUC variables, but I wonder if we want to
> > save the value of each one at subtransaction start and recover it at
> > abort? Things could easily get huge. Maybe only saving the ones that
> > are different from the default value, and from the last saved value.
>
> We have an on-commit field in the guc structures to handle
> commit/rollback settings. Do we need to extend that to subtransactions?

Yes IMHO. I'm not sure actually _how_ should this be handled. Maybe
the on-commit field should go away and be replaced by something more
general (probably a stack, like everything else). Let me look at the
code.

> I don't think you can save off only the defaults in an efficient manner.

What do you mean by efficient? Space efficient? It will be much more
efficient to save only the changed values. Time efficient? We will
have to traverse the whole list anyway, whether we only save the changed
values or all of them.

Remember, we already traverse the whole list of shared buffers, the
whole CatCache, the whole Relcache, maybe do some repallocs, and lots of
other stuff. Traversing the whole GUC array does not seem all that
expensive to me ... After all, we are saving lots of I/O if subxacts
are used correctly (try, rollback, try again -- you save the XLog for
the first try.)

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

Alvaro Herrera wrote:
> On Fri, May 28, 2004 at 11:11:27PM -0400, Bruce Momjian wrote:
> > Alvaro Herrera wrote:
>
> > > I think this applies to all GUC variables, but I wonder if we want to
> > > save the value of each one at subtransaction start and recover it at
> > > abort? Things could easily get huge. Maybe only saving the ones that
> > > are different from the default value, and from the last saved value.
> >
> > We have an on-commit field in the guc structures to handle
> > commit/rollback settings. Do we need to extend that to subtransactions?
>
> Yes IMHO. I'm not sure actually _how_ should this be handled. Maybe
> the on-commit field should go away and be replaced by something more
> general (probably a stack, like everything else). Let me look at the
> code.
>
> > I don't think you can save off only the defaults in an efficient manner.
>
> What do you mean by efficient? Space efficient? It will be much more
> efficient to save only the changed values. Time efficient? We will
> have to traverse the whole list anyway, whether we only save the changed
> values or all of them.
>
> Remember, we already traverse the whole list of shared buffers, the
> whole CatCache, the whole Relcache, maybe do some repallocs, and lots of
> other stuff. Traversing the whole GUC array does not seem all that
> expensive to me ... After all, we are saving lots of I/O if subxacts
> are used correctly (try, rollback, try again -- you save the XLog for
> the first try.)

My comment was based on the fact that guc already does some special
_saves_ when you change a value and triggers some stuff on xact end. I
was just thinking it would be cleaner to use that infrastructure rather
than do a scan not knowing if any GUC will change or not, but if a scan
is easier, I think that would be fine.

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

On Fri, 28 May 2004, Alvaro Herrera wrote:

> On Fri, May 28, 2004 at 05:43:41PM -0700, Stephan Szabo wrote:
>
> > On Wed, 26 May 2004, Alvaro Herrera wrote:
> >
> > > I have tested it and it passes all regression tests (including ones I
> > > added), plus some more tests I threw at it mainly for concurrency.
> > > Everything behaves as expected. At this time I'd like to have it
> > > reviewed by the critic eye of the committers, and tested by whoever
> > > would be using it.
> >
> > I unfortunately didn't really follow the discussions in the past (sorry :(
> > ), but are the transaction state modifying statements done in a
> > subtransaction supposed to live beyond subtransaction rollback?
>
> Hmm, I suppose not.

Actually, looking at it, for set transaction, the worse one is probably
isolation level because it can't be set after a query has been run and I'm
not entirely sure what the behavior should look like if you did something
like:

begin;
set transaction isolation level serializable;
begin;
set transaction isolation level read committed;
select * from a;
rollback;

At that point, is the transaction in serializable method and is the
snapshot set even though the select "never happened" and what does the
above really mean. Is it that the results of that first transactions are
out of serializability with the rest of the transaction?

Also related, although START TRANSACTION (specifying isolation level or
read onlyness as part) is currently defined to act as if set transaction
was used, it seems really odd that the settings would leak to the outer
translation even on a commit and that you can't specify isolation level -
even if it's the same level - if the outer transaction has done any
queries.

-----

BTW: For the deferred trigger stuff, I am guessing you haven't touched
that at all in the current patch?

I wonder if the following would work assuming that we want deferred
triggers to run at outer transaction end?

--
There's one trigger queue list and a stack of "last event before this
(sub)transaction started" pointers in and a current one similar to _imm is
the one for the current subtransaction. Scanning the list takes an extra
argument to say whether to consolidate the list or not (generally true
except for set state).

On outer transaction start,
NULL is set as the last event at start pointer

On subtransaction start,
The current last event at start pointer is added to the stack.
The last event at start pointer is set to _imm.
The _imm pointer should already be in the correct place

On subtransaction abort,
The queue is deallocated after the last event at start pointer and
the event in question its next pointer reset.
(if null, the list is entirely deallocated and the list is
set as null)
The _imm pointer is set to the last event at start pointer.
The last event at start pointer is popped from the stack.

On outer transaction abort,
Clear the queue.

On subtransaction commit,
The last event at start pointer is popped from the stack.

On outer transaction commit
Scan the deferred events.

On set state,
We call DeferredTriggerInvokeEvents with (true, false) rather than
relying on the statement end to do it (as true, true). This should set
the _imm pointer so that the end of statement won't scan anything anyway.

--
I think it might be possible to do the queue deallocation for
subtransaction abort with appropriate context work (each one gets a
context under its parent's and on abort it's removed and on commit it's
not until you reach the outermost?) but I haven't though about it enough.
And I think the non-removing events on set transaction could be fixed as
well.

On Sat, May 29, 2004 at 08:25:27AM -0700, Stephan Szabo wrote:

> Also related, although START TRANSACTION (specifying isolation level or
> read onlyness as part) is currently defined to act as if set transaction
> was used, it seems really odd that the settings would leak to the outer
> translation even on a commit and that you can't specify isolation level -
> even if it's the same level - if the outer transaction has done any
> queries.

Hmm ... isolation level and read onlyness was discussed last year and I
think we had a working design. I'll look into my archives.

> BTW: For the deferred trigger stuff, I am guessing you haven't touched
> that at all in the current patch?
>
> I wonder if the following would work assuming that we want deferred
> triggers to run at outer transaction end?

Ah, this seems to work. I'll implement it and I'll let you know how it
goes.

> I think it might be possible to do the queue deallocation for
> subtransaction abort with appropriate context work (each one gets a
> context under its parent's and on abort it's removed and on commit it's
> not until you reach the outermost?) but I haven't though about it enough.

Actually there already is such a global context and I think it's appropiate
here.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

On Sat, 29 May 2004, Alvaro Herrera wrote:

> On Sat, May 29, 2004 at 08:25:27AM -0700, Stephan Szabo wrote:
>
> > Also related, although START TRANSACTION (specifying isolation level or
> > read onlyness as part) is currently defined to act as if set transaction
> > was used, it seems really odd that the settings would leak to the outer
> > translation even on a commit and that you can't specify isolation level -
> > even if it's the same level - if the outer transaction has done any
> > queries.
>
> Hmm ... isolation level and read onlyness was discussed last year and I
> think we had a working design. I'll look into my archives.
>
>
> > BTW: For the deferred trigger stuff, I am guessing you haven't touched
> > that at all in the current patch?
> >
> > I wonder if the following would work assuming that we want deferred
> > triggers to run at outer transaction end?
>
> Ah, this seems to work. I'll implement it and I'll let you know how it
> goes.

Note, that I think you'd still need to do some kind of tiered thing for
set constraint status, but I didn't think about that yet.

> > I think it might be possible to do the queue deallocation for
> > subtransaction abort with appropriate context work (each one gets a
> > context under its parent's and on abort it's removed and on commit it's
> > not until you reach the outermost?) but I haven't though about it enough.
>
> Actually there already is such a global context and I think it's appropiate
> here.

We probably want to be able to easily remove all the items for an aborted
sub transaction, and I was hoping that there might be an easy way to use
the contexts to do that rather than manually trolling through deallocating
entries from the queue.

On Sat, 29 May 2004, Alvaro Herrera wrote:

> On Sat, May 29, 2004 at 08:25:27AM -0700, Stephan Szabo wrote:
>
> > BTW: For the deferred trigger stuff, I am guessing you haven't touched
> > that at all in the current patch?
> >
> > I wonder if the following would work assuming that we want deferred
> > triggers to run at outer transaction end?
>
> Ah, this seems to work. I'll implement it and I'll let you know how it
> goes.

Ugh... There's one further wrinkle I hadn't thought about, imagine the
following:

begin;
-- here the transaction does something that makes deferred trigger
-- entries
begin;
set constraints all immediate;
-- we now run through doing the deferred trigger items
rollback;
-- we need to unmark that the deferred items from the
-- outer transaction have been run. However, in general,
-- it might not be all entries nor all entries that are marked
-- as done.

I'm not sure how expensive it is to check if a given subxact has
committed, but maybe instead of just done/not done, we need to say
something like what xid marked the trigger and instead of

if (!(event->dte_event & (TRIGGER_DEFERRED_DONE |
TRIGGER_DEFERRED_CANCELED)))

inside deferredTriggerInvokeEvents we do something like:

if (!((event->dte_event & (TRIGGER_DEFERRED_DONE |
TRIGGER_DEFERRED_CANCELED)) && /*something to check that the marking xact
is either myself or a committed subxact*/))

On 5/28/2004 2:52 PM, Alvaro Herrera wrote:

> On Fri, May 28, 2004 at 01:43:16PM -0400, Bruce Momjian wrote:
>
>> In this case, I want to try all of the inserts, but any of them can
>> fail, then I want the bottom part done.
>
> I wonder where everyone eas when I asked this question a lot of time
> ago. I said I thought the behavior should be like I described, and no
> one objected.
>
> Personally I think it would be a mistake to allow the COMMIT for the
> subtransaction to ignore the fact that the subxact was aborted. However
> I realize what you are proposing, and maybe this can be implemented
> using a parameter to COMMIT (indicating to not propagate the error if
> it's in aborted state, but commit normally otherwise).

I agree on this one. Subtransactions are a feature to add more fine
control to applications, not to ignore error checking for scripting.

>
> However if everyone disagrees, I can take that part out, and the code
> would be simpler. IMHO however, it would be less reliable.

Please don't.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck(at)Yahoo(dot)com #

On Sun, May 30, 2004 at 04:07:27AM -0700, Stephan Szabo wrote:
> On Sat, 29 May 2004, Alvaro Herrera wrote:

> > Ah, this seems to work. I'll implement it and I'll let you know how it
> > goes.
>
> Ugh... There's one further wrinkle I hadn't thought about, imagine the
> following:

Ok Stephan, thank you very much for your help. I implemented this and
it seems to work, at least on my (limited?) test scenario. If you have
some spare time I'd like you to test it and see if you can break it (I
posted it to -patches yesterday).

Or please see my test case below. Is it missing something? Note that
if I take out any of the four inserts just before the end of the
transaction, the whole thing is rejected.

DROP TABLE foo CASCADE;
DROP TABLE bar CASCADE;
CREATE TABLE foo (A INT UNIQUE);
CREATE TABLE bar (A INT REFERENCES foo(A) DEFERRABLE);

DELETE FROM bar;
DELETE FROM foo;
INSERT INTO foo VALUES (1);
INSERT INTO foo VALUES (2);
BEGIN;
SET CONSTRAINTS ALL DEFERRED;
INSERT INTO bar VALUES (1);
BEGIN;
INSERT INTO bar VALUES (3);
COMMIT;
BEGIN;
BEGIN;
INSERT INTO bar VALUES (4);
COMMIT;
INSERT INTO foo VALUES (3);
SET CONSTRAINTS ALL IMMEDIATE;
ROLLBACK;
SET CONSTRAINTS ALL DEFERRED;
BEGIN;
INSERT INTO bar VALUES (5);
COMMIT;
BEGIN;
BEGIN;
INSERT INTO bar VALUES (6);
ROLLBACK;
COMMIT;
BEGIN;
INSERT INTO bar VALUES (7);
COMMIT;
BEGIN;
BEGIN;
INSERT INTO bar VALUES (9);
COMMIT;
COMMIT;
INSERT INTO foo VALUES(3);
INSERT INTO foo VALUES(5);
INSERT INTO foo VALUES(7);
INSERT INTO foo VALUES(9);
COMMIT;

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Acepta los honores y aplausos y perderás tu libertad"

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.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Ciencias políticas es la ciencia de entender por qué
los políticos actúan como lo hacen" (netfunny.com)

Alvaro Herrera wrote:
> > 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

Perfect. Your suggested behavior is best. I think I like "COMMIT
IGNORE ABORT" best, but we can disucss this some more.

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

On Wed, 9 Jun 2004, Alvaro Herrera wrote:

> On Sun, May 30, 2004 at 04:07:27AM -0700, Stephan Szabo wrote:
> > On Sat, 29 May 2004, Alvaro Herrera wrote:
>
> > > Ah, this seems to work. I'll implement it and I'll let you know how it
> > > goes.
> >
> > Ugh... There's one further wrinkle I hadn't thought about, imagine the
> > following:
>
> Ok Stephan, thank you very much for your help. I implemented this and
> it seems to work, at least on my (limited?) test scenario. If you have
> some spare time I'd like you to test it and see if you can break it (I
> posted it to -patches yesterday).

Unfortunately, I've gotten it to fail, but I haven't looked in depth (I'm
at work, so I'm doing it during compilations and such.)

I made a file ins with a 1000000 line copy to a table named fk with the
value 1, and then if I do the following:

create table pk(a int primary key);
create table fk(a int references pk(a) initially deferred);
insert into pk values (1);

begin;
begin;
\i ins
\i ins
rollback;
\i ins
commit;

It appears to exception on the third \i ins with the tail_thisxact
pointing into a bad place.

On Wed, 9 Jun 2004, Stephan Szabo wrote:

> On Wed, 9 Jun 2004, Alvaro Herrera wrote:
>
> > On Sun, May 30, 2004 at 04:07:27AM -0700, Stephan Szabo wrote:
> > > On Sat, 29 May 2004, Alvaro Herrera wrote:
> >
> > > > Ah, this seems to work. I'll implement it and I'll let you know how it
> > > > goes.
> > >
> > > Ugh... There's one further wrinkle I hadn't thought about, imagine the
> > > following:
> >
> > Ok Stephan, thank you very much for your help. I implemented this and
> > it seems to work, at least on my (limited?) test scenario. If you have
> > some spare time I'd like you to test it and see if you can break it (I
> > posted it to -patches yesterday).
>
> Unfortunately, I've gotten it to fail, but I haven't looked in depth (I'm
> at work, so I'm doing it during compilations and such.)
>
> I made a file ins with a 1000000 line copy to a table named fk with the
> value 1, and then if I do the following:
>
> create table pk(a int primary key);
> create table fk(a int references pk(a) initially deferred);
> insert into pk values (1);
>
> begin;
> begin;
> \i ins
> \i ins
> rollback;
> \i ins
> commit;
>
> It appears to exception on the third \i ins with the tail_thisxact
> pointing into a bad place.

Okay - I think I see what's going on here.

It looks like deferredTriggerInvokeEvents is being run (immediate_only),
but since deferredTriggers->events_imm is NULL it's using
deferredTriggers->events as the start of the list to check, but this value
isn't getting reset in DeferredTriggerEndSubXact in the case that the
entire list was created in an aborted subtransaction.

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

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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> 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:

While we clearly want this functionality, I tend to agree with Barry
that COMMIT IGNORE ABORT (and the other variants that have been floated)
is a horrid, confusing name for it. I would suggest using END with some
modifier, instead. Perhaps

END [ WORK | TRANSACTION ] [ IGNORE ERRORS ]

END doesn't so directly imply that you are trying to commit a failed
transaction.

regards, tom lane

On Thu, Jun 10, 2004 at 03:39:14PM -0400, Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > 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:
>
> While we clearly want this functionality, I tend to agree with Barry
> that COMMIT IGNORE ABORT (and the other variants that have been floated)
> is a horrid, confusing name for it. I would suggest using END with some
> modifier, instead. Perhaps
>
> END [ WORK | TRANSACTION ] [ IGNORE ERRORS ]
>
> END doesn't so directly imply that you are trying to commit a failed
> transaction.

The problem with END is how about executing it inside a PL/pgSQL
function. Can we distinguish it from plpgsql's END?

Also, COMMITing an aborted main transaction is the same as ENDing it;
and in fact, it's the same as ROLLBACK. Why is it more confusing for a
subtransaction to behave the same?

I agree that the grammar I proposed is wrong. I guess I can ask for two
words and then strcmp() them to "ignore errors"?

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La naturaleza, tan frágil, tan expuesta a la muerte... y tan viva"

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> On Thu, Jun 10, 2004 at 03:39:14PM -0400, Tom Lane wrote:
>> END doesn't so directly imply that you are trying to commit a failed
>> transaction.

> The problem with END is how about executing it inside a PL/pgSQL
> function. Can we distinguish it from plpgsql's END?

We're going to have to deal with that on the BEGIN side anyway.
A reasonable possibility would be to require the TRANSACTION word
to appear when you do it in plpgsql.

> Also, COMMITing an aborted main transaction is the same as ENDing it;
> and in fact, it's the same as ROLLBACK. Why is it more confusing for a
> subtransaction to behave the same?

But the point here is that the behavior would *not* be the same.

regards, tom lane

On Wed, Jun 09, 2004 at 11:32:08PM -0700, Stephan Szabo wrote:

> > Unfortunately, I've gotten it to fail, but I haven't looked in depth (I'm
> > at work, so I'm doing it during compilations and such.)

[...]

> Okay - I think I see what's going on here.
>
> It looks like deferredTriggerInvokeEvents is being run (immediate_only),
> but since deferredTriggers->events_imm is NULL it's using
> deferredTriggers->events as the start of the list to check, but this value
> isn't getting reset in DeferredTriggerEndSubXact in the case that the
> entire list was created in an aborted subtransaction.

Ok, thanks for the test and diagnostics; patch attached. I'll see if I
can find other situations like this.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"No hay cielo posible sin hundir nuestras raíces
en la profundidad de la tierra" (Malucha Pinto)

On Thu, 10 Jun 2004, Alvaro Herrera wrote:

> On Wed, Jun 09, 2004 at 11:32:08PM -0700, Stephan Szabo wrote:
>
> > > Unfortunately, I've gotten it to fail, but I haven't looked in depth (I'm
> > > at work, so I'm doing it during compilations and such.)
>
> [...]
>
> > Okay - I think I see what's going on here.
> >
> > It looks like deferredTriggerInvokeEvents is being run (immediate_only),
> > but since deferredTriggers->events_imm is NULL it's using
> > deferredTriggers->events as the start of the list to check, but this value
> > isn't getting reset in DeferredTriggerEndSubXact in the case that the
> > entire list was created in an aborted subtransaction.
>
> Ok, thanks for the test and diagnostics; patch attached. I'll see if I
> can find other situations like this.

As a question, what was the general assumption about what the following
should do (using a modification of the original test case)?

DROP TABLE foo CASCADE;
DROP TABLE bar CASCADE;
CREATE TABLE foo (A INT UNIQUE);
CREATE TABLE bar (A INT REFERENCES foo(A) DEFERRABLE);

DELETE FROM bar;
DELETE FROM foo;
INSERT INTO foo VALUES (1);
INSERT INTO foo VALUES (2);
BEGIN;
SET CONSTRAINTS ALL DEFERRED;
INSERT INTO bar VALUES (1);
BEGIN;
INSERT INTO bar VALUES (3);
COMMIT;
BEGIN;
BEGIN;
INSERT INTO bar VALUES (4);
COMMIT;
INSERT INTO foo VALUES (3);
SET CONSTRAINTS ALL IMMEDIATE;
ROLLBACK;
-- move this set constraints down below
-- SET CONSTRAINTS ALL DEFERRED;
BEGIN;
INSERT INTO bar VALUES (5); --(1)
COMMIT;
SET CONSTRAINTS ALL DEFERRED;
BEGIN;
BEGIN;
INSERT INTO bar VALUES (6);
ROLLBACK;
COMMIT;
BEGIN;
INSERT INTO bar VALUES (7);
COMMIT;
BEGIN;
BEGIN;
INSERT INTO bar VALUES (9);
COMMIT;
COMMIT;
INSERT INTO foo VALUES(3);
INSERT INTO foo VALUES(5);
INSERT INTO foo VALUES(7);
INSERT INTO foo VALUES(9);
COMMIT;

Should the statement at (1) fail because the constraint is not immediately
satisfied, or should it pass because the set constraints all immediate was
part of a subtransaction that didn't commit?

On Sat, 12 Jun 2004, Alvaro Herrera wrote:

> On Sat, Jun 12, 2004 at 10:27:15AM -0700, Stephan Szabo wrote:
>
> > As a question, what was the general assumption about what the following
> > should do (using a modification of the original test case)?
>
> [...]
>
> > Should the statement at (1) fail because the constraint is not immediately
> > satisfied, or should it pass because the set constraints all immediate was
> > part of a subtransaction that didn't commit?
>
> I think the correct answer should be that (1) should pass because the
> all immediate set would be rolled back. However, GUC vars do not
> presently roll back so it doesn't work. In fact, if GUC vars do roll
> back, then there is no need to explicitly set all constraints to
> deferred, because it would roll back to that value automatically.
>
> GUC vars are the remaining item for correct nested xacts AFAICS.

AFAIK, SET CONSTRAINTS isn't modifying a GUC variable, it gets turned into
a command that goes to DeferredTriggerSetState in trigger.c.