bug w/ cursors and savepoints
Someone at Fujitsu pointed out the following bug in 8.0:
begin;
savepoint x;
create table abc (a int);
insert into abc values (5);
declare foo cursor for select * from abc;
rollback to x;
fetch from foo; -- hits an Assert()
commit;
The stacktrace is:
#2 0x0826367b in ExceptionalCondition (conditionName=0x8316544
"!(((bool)((relation)->rd_refcnt == 0)))",
errorType=0x8316004 "FailedAssertion", fileName=0x8315f08
"/home/neilc/pgsql/src/backend/utils/cache/relcache.c", lineNumber=2118)
at /home/neilc/pgsql/src/backend/utils/error/assert.c:51
#3 0x0825cec0 in AtEOSubXact_RelationCache (isCommit=0 '\0', mySubid=2,
parentSubid=1)
at /home/neilc/pgsql/src/backend/utils/cache/relcache.c:2118
#4 0x080ade30 in AbortSubTransaction ()
at /home/neilc/pgsql/src/backend/access/transam/xact.c:3407
#5 0x080ac404 in CommitTransactionCommand ()
at /home/neilc/pgsql/src/backend/access/transam/xact.c:1982
#6 0x081de4ba in finish_xact_command ()
at /home/neilc/pgsql/src/backend/tcop/postgres.c:1843
#7 0x081dd102 in exec_simple_query (query_string=0x83b6ad4 "rollback to
x;") at /home/neilc/pgsql/src/backend/tcop/postgres.c:950
So what's happening is that the cursor still holds a reference to the
newly-created table, so we can't just blow it away. I don't know the
subtransaction code too well, so I'm not sure of the right fix.
Comments?
-Neil
Neil Conway <neilc@samurai.com> writes:
Someone at Fujitsu pointed out the following bug in 8.0:
begin;
savepoint x;
create table abc (a int);
insert into abc values (5);
declare foo cursor for select * from abc;
rollback to x;
fetch from foo; -- hits an Assert()
Offhand I'd say this should draw a "no such cursor as foo" error.
I'm too tired to look into why foo still exists after the rollback...
regards, tom lane
On Tue, 2005-01-25 at 02:40 -0500, Tom Lane wrote:
Offhand I'd say this should draw a "no such cursor as foo" error.
I'm too tired to look into why foo still exists after the rollback...
I'm confused; I wasn't involved in the design discussions about portals
and subtransactions this summer, but my understanding is that making
portals non-transactional was the conclusion. Shouldn't that imply that
a DECLARE in an aborted subtransaction should persist? Certainly, it
seems AtSubAbort_Portals() makes sure that READY portals created in an
aborted subtxn are adopted by the parent transaction.
(It might make sense to persist the cursor to the parent transaction,
unless the aborted transaction created a database object the cursor
depends upon, a la [1]http://archives.postgresql.org/pgsql-hackers/2004-07/msg00349.php -- but in that case, IMHO we ought to give a
different error message than "no such cursor".)
If someone can shed some light on what the spec for this behavior ought
to be, I'd be happy to fix the bug.
-Neil
[1]: http://archives.postgresql.org/pgsql-hackers/2004-07/msg00349.php
Neil Conway <neilc@samurai.com> writes:
On Tue, 2005-01-25 at 02:40 -0500, Tom Lane wrote:
Offhand I'd say this should draw a "no such cursor as foo" error.
I'm too tired to look into why foo still exists after the rollback...
I'm confused; I wasn't involved in the design discussions about portals
and subtransactions this summer, but my understanding is that making
portals non-transactional was the conclusion. Shouldn't that imply that
a DECLARE in an aborted subtransaction should persist?
I don't recall the discussions from last summer in detail, but it can't
possibly be rational to allow a cursor created in a failed
subtransaction to persist beyond that subtransaction... your example
in which the cursor uses tables that no longer exist is a fairly
egregious example of why not, but there are others.
regards, tom lane
On Tue, Jan 25, 2005 at 02:40:51AM -0500, Tom Lane wrote:
Neil Conway <neilc@samurai.com> writes:
Someone at Fujitsu pointed out the following bug in 8.0:
begin;
savepoint x;
create table abc (a int);
insert into abc values (5);
declare foo cursor for select * from abc;
rollback to x;
fetch from foo; -- hits an Assert()Offhand I'd say this should draw a "no such cursor as foo" error.
I'm too tired to look into why foo still exists after the rollback...
At this point, gdb says that the portal is in PORTAL_READY state. The
code says to keep it open and reassign it to the parent subxact. I
don't remember what the rationale for this was ... I'll review the
discussion about this.
--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"La rebeld�a es la virtud original del hombre" (Arthur Schopenhauer)
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
On Tue, Jan 25, 2005 at 02:40:51AM -0500, Tom Lane wrote:
Offhand I'd say this should draw a "no such cursor as foo" error.
I'm too tired to look into why foo still exists after the rollback...
At this point, gdb says that the portal is in PORTAL_READY state. The
code says to keep it open and reassign it to the parent subxact. I
don't remember what the rationale for this was ... I'll review the
discussion about this.
IIRC, we had agreed that in a sequence like
BEGIN;
DECLARE c CURSOR ...;
SAVEPOINT x;
FETCH FROM c;
ROLLBACK TO x;
FETCH FROM c;
...
the second fetch should get the second cursor row, ie, the rollback does
not undo the cursor state change caused by the first fetch. This was
frankly driven mostly by implementation considerations, ie, we do not
have any mechanism that would support undoing changes of Executor state.
But I don't think that we really thought about the case of rolling back
the *creation* of a cursor. Neil's example seems to prove that we need
to do that. Even aside from the possibility that the database objects
won't exist any more, the locks taken out during plan startup would get
released by the rollback. So I think the rule ought to be that cursors
created inside a failed subtransaction go away.
I have some recollection that we did that initially and ran into the
problem that the portal holding the ROLLBACK command itself goes away
too soon :-(. So a bit of care will be needed here.
regards, tom lane
I wrote:
So I think the rule ought to be that cursors
created inside a failed subtransaction go away.
Other bits of recollection bubbling up: I think that one reason we made
portals not go away instantly on error is that the JDBC guys objected,
feeling that it made for weird special cases at the protocol level.
So the right fix might involve putting the portal into PORTAL_FAILED
state rather than just zapping it completely.
regards, tom lane
On Tue, Jan 25, 2005 at 12:32:57PM -0500, Tom Lane wrote:
So the right fix might involve putting the portal into PORTAL_FAILED
state rather than just zapping it completely.
Strangely, the code comes up simpler after the fix. Patch attached.
Regression test pass. Additionaly I tried both cases mentioned in this
thread; maybe it's worthy to add tests for them too.
alvherre=# begin;
BEGIN
alvherre=# savepoint x;
SAVEPOINT
alvherre=# create table abc (a int);
CREATE TABLE
alvherre=# insert into abc values (5);
INSERT 33616 1
alvherre=# declare foo cursor for select * from abc;
DECLARE CURSOR
alvherre=# rollback to x;
ROLLBACK
alvherre=# fetch from foo; -- hits an Assert()
ERROR: no existe el cursor �foo�
alvherre=# commit;
ROLLBACK
alvherre=# begin;
BEGIN
alvherre=# declare c cursor for select 1 union all select 2;
DECLARE CURSOR
alvherre=# savepoint x;
SAVEPOINT
alvherre=# fetch from c;
?column?
----------
1
(1 fila)
alvherre=# rollback to x;
ROLLBACK
alvherre=# fetch from c;
?column?
----------
2
(1 fila)
alvherre=# commit;
COMMIT
--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"The ability to monopolize a planet is insignificant
next to the power of the source"
Attachments:
cursor-fix.patchtext/plain; charset=us-asciiDownload
Index: portalmem.c
===================================================================
RCS file: /home/alvherre/cvs/pgsql/src/backend/utils/mmgr/portalmem.c,v
retrieving revision 1.76
diff -c -r1.76 portalmem.c
*** portalmem.c 31 Dec 2004 22:02:48 -0000 1.76
--- portalmem.c 25 Jan 2005 17:52:52 -0000
***************
*** 623,663 ****
continue;
/*
! * Force any active portals of my own transaction into FAILED
* state. This is mostly to ensure that a portal running a FETCH
* will go FAILED if the underlying cursor fails. (Note we do NOT
* want to do this to upper-level portals, since they may be able
* to continue.)
*/
! if (portal->status == PORTAL_ACTIVE)
portal->status = PORTAL_FAILED;
! /*
! * If the portal is READY then allow it to survive into the parent
! * transaction; otherwise shut it down.
! */
! if (portal->status == PORTAL_READY)
! {
! portal->createSubid = parentSubid;
! if (portal->resowner)
! ResourceOwnerNewParent(portal->resowner, parentXactOwner);
! }
! else
{
! /* let portalcmds.c clean up the state it knows about */
! if (PointerIsValid(portal->cleanup))
! {
! (*portal->cleanup) (portal);
! portal->cleanup = NULL;
! }
!
! /*
! * Any resources belonging to the portal will be released in
! * the upcoming transaction-wide cleanup; they will be gone
! * before we run PortalDrop.
! */
! portal->resowner = NULL;
}
}
}
--- 623,650 ----
continue;
/*
! * Force any active and ready portals of my own transaction into FAILED
* state. This is mostly to ensure that a portal running a FETCH
* will go FAILED if the underlying cursor fails. (Note we do NOT
* want to do this to upper-level portals, since they may be able
* to continue.)
*/
! if (portal->status == PORTAL_ACTIVE || portal->status == PORTAL_READY)
portal->status = PORTAL_FAILED;
! /* let portalcmds.c clean up the state it knows about */
! if (PointerIsValid(portal->cleanup))
{
! (*portal->cleanup) (portal);
! portal->cleanup = NULL;
}
+
+ /*
+ * Any resources belonging to the portal will be released in
+ * the upcoming transaction-wide cleanup; they will be gone
+ * before we run PortalDrop.
+ */
+ portal->resowner = NULL;
}
}
On Tue, Jan 25, 2005 at 03:14:07PM -0300, Alvaro Herrera wrote:
On Tue, Jan 25, 2005 at 12:32:57PM -0500, Tom Lane wrote:
So the right fix might involve putting the portal into PORTAL_FAILED
state rather than just zapping it completely.Strangely, the code comes up simpler after the fix. Patch attached.
I forgot to mention that I looked at AtAbort_Portals and that while it
has the same test to change the state of an active portal, it's probably
safe to assume that no cursor will be allowed to run from that point on.
Now that I think of it, maybe it's a good idea to add a comment saying
why is that the case.
--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"When the proper man does nothing (wu-wei),
his thought is felt ten thousand miles." (Lao Tse)
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
! if (portal->status == PORTAL_ACTIVE)
portal->status = PORTAL_FAILED;
! if (portal->status == PORTAL_ACTIVE || portal->status == PORTAL_READY)
portal->status = PORTAL_FAILED;
I don't think you actually need that change, since you're going to
unconditionally close the portal below. The case for PORTAL_ACTIVE
is just there to dodge the sanity check in PortalDrop.
The routine's comments need a bit of work too. Otherwise it seems OK.
Neil or anyone else --- see an issue here?
regards, tom lane
Tom Lane wrote:
The routine's comments need a bit of work too. Otherwise it seems OK.
Neil or anyone else --- see an issue here?
The policy will now be: cursor creation is transaction, but cursor state
modifications (FETCH) are non-transactional -- right? I wonder if it
wouldn't be more consistent to make cursor deletion (CLOSE)
transactional as well -- so that a CLOSE in an aborted subtransaction
would not actually destroy the cursor.
Other than that, I think there ought to be some user-level documentation
for how cursors and savepoints interact, and some regression tests for
this behavior, but I'm happy to add that myself if no one beats me to it.
-Neil
On Wed, Jan 26, 2005 at 03:33:07PM +1100, Neil Conway wrote:
Tom Lane wrote:
The routine's comments need a bit of work too. Otherwise it seems OK.
Neil or anyone else --- see an issue here?The policy will now be: cursor creation is transaction, but cursor state
modifications (FETCH) are non-transactional -- right? I wonder if it
wouldn't be more consistent to make cursor deletion (CLOSE)
transactional as well -- so that a CLOSE in an aborted subtransaction
would not actually destroy the cursor.
Hmm ... not sure how hard that is. We left a lot of details for 8.1
though, like trying to save the state of the executor related to the
cursor so that FETCH is transactional too.
Other than that, I think there ought to be some user-level documentation
for how cursors and savepoints interact,
There is some detail (as of my patch, outdated) in
http://developer.postgresql.org/docs/postgres/sql-rollback-to.html
If you have a suggestion on where else it should go I'm all ears ...
and some regression tests for this behavior, but I'm happy to add that
myself if no one beats me to it.
Please do.
I'll post a corrected patch ASAP, including the doc change.
--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"La espina, desde que nace, ya pincha" (Proverbio africano)
On Tue, Jan 25, 2005 at 02:06:24PM -0300, Alvaro Herrera wrote:
Hackers,
At this point, gdb says that the portal is in PORTAL_READY state. The
code says to keep it open and reassign it to the parent subxact. I
don't remember what the rationale for this was ... I'll review the
discussion about this.
Our conclusion at the time was that cursors created inside failed
subtransactions should remain open. See the proposal and followup
discussion starting here:
http://archives.postgresql.org/pgsql-hackers/2004-07/msg00700.php
If we want to keep this behavior then we should not close all READY
cursors as per my previous patch. We should keep them open, and only
mark FAILED those cursors that are related to state reversed by the
aborting subtransaction.
I don't see any way to do this cleanly, until we have some relcache-
dependency checking on Portals (maybe anything else?). Not sure what we
can do about it right now, with 8.0.1 release tomorrow.
--
Alvaro Herrera (<alvherre[@]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 <alvherre@dcc.uchile.cl> writes:
Our conclusion at the time was that cursors created inside failed
subtransactions should remain open. See the proposal and followup
discussion starting here:
http://archives.postgresql.org/pgsql-hackers/2004-07/msg00700.php
If we want to keep this behavior then we should not close all READY
cursors as per my previous patch. We should keep them open, and only
mark FAILED those cursors that are related to state reversed by the
aborting subtransaction.
I don't see any way to do this cleanly, until we have some relcache-
dependency checking on Portals (maybe anything else?). Not sure what we
can do about it right now, with 8.0.1 release tomorrow.
I don't think we have a lot of choices: we have to destroy (or at least
mark FAILED) all such cursors for the time being. The whole question of
cursor transaction semantics could stand to be revisited in 8.1, but we
can't ship the system with such a trivial crashing bug.
Note that dependency tracking would not in itself be enough to solve the
problem, since the question is not merely what objects the cursor
depends on, but whether their definitions were changed in the failed
subtransaction. Talk about messy :-(
regards, tom lane
On Wed, Jan 26, 2005 at 05:10:09PM -0500, Tom Lane wrote:
I don't think we have a lot of choices: we have to destroy (or at least
mark FAILED) all such cursors for the time being.
I don't see a lot of difference between marking the portal FAILED and
destroying it (maybe I'm looking at the wrong code). So I just took the
simpler approach; patch attached.
Note that dependency tracking would not in itself be enough to solve the
problem, since the question is not merely what objects the cursor
depends on, but whether their definitions were changed in the failed
subtransaction. Talk about messy :-(
Maybe we can use the shared cache invalidation mechanism for this. When
a message is received that affects a relation marked as referenced by a
portal, mark the portal obsolete. I don't recall the details of
shared-inval right now, I may be missing something (like the time at
which messages are sent. But I believe we send a message to our own
backend, no?)
--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"Always assume the user will do much worse than the stupidest thing
you can imagine." (Julien PUYDT)
Attachments:
cursor-fix.patchtext/plain; charset=us-asciiDownload
Index: doc/src/sgml/ref/rollback_to.sgml
===================================================================
RCS file: /home/alvherre/cvs/pgsql/doc/src/sgml/ref/rollback_to.sgml,v
retrieving revision 1.5
diff -c -r1.5 rollback_to.sgml
*** doc/src/sgml/ref/rollback_to.sgml 27 Nov 2004 21:27:07 -0000 1.5
--- doc/src/sgml/ref/rollback_to.sgml 26 Jan 2005 21:15:18 -0000
***************
*** 74,81 ****
<para>
Cursors have somewhat non-transactional behavior with respect to
! savepoints. Any cursor that is opened inside the savepoint is not closed
! when the savepoint is rolled back. If a cursor is affected by a
<command>FETCH</> command inside a savepoint that is later rolled
back, the cursor position remains at the position that <command>FETCH</>
left it pointing to (that is, <command>FETCH</> is not rolled back).
--- 74,83 ----
<para>
Cursors have somewhat non-transactional behavior with respect to
! savepoints. Any cursor that is opened inside the savepoint is closed
! when the savepoint is rolled back, and a cursor that is closed inside
! the savepoint remains closed if the savepoint is rolled back.
! If a cursor is affected by a
<command>FETCH</> command inside a savepoint that is later rolled
back, the cursor position remains at the position that <command>FETCH</>
left it pointing to (that is, <command>FETCH</> is not rolled back).
Index: src/backend/utils/mmgr/portalmem.c
===================================================================
RCS file: /home/alvherre/cvs/pgsql/src/backend/utils/mmgr/portalmem.c,v
retrieving revision 1.76
diff -c -r1.76 portalmem.c
*** src/backend/utils/mmgr/portalmem.c 31 Dec 2004 22:02:48 -0000 1.76
--- src/backend/utils/mmgr/portalmem.c 26 Jan 2005 22:36:17 -0000
***************
*** 601,609 ****
/*
* Subtransaction abort handling for portals.
*
! * Deactivate failed portals created during the failed subtransaction.
* Note that per AtSubCommit_Portals, this will catch portals created
* in descendants of the subtransaction too.
*/
void
AtSubAbort_Portals(SubTransactionId mySubid,
--- 601,612 ----
/*
* Subtransaction abort handling for portals.
*
! * Deactivate portals created during the failed subtransaction.
* Note that per AtSubCommit_Portals, this will catch portals created
* in descendants of the subtransaction too.
+ *
+ * Note that we don't destroy any portals here; that's done in
+ * AtSubCleanup_Portals.
*/
void
AtSubAbort_Portals(SubTransactionId mySubid,
***************
*** 628,663 ****
* will go FAILED if the underlying cursor fails. (Note we do NOT
* want to do this to upper-level portals, since they may be able
* to continue.)
*/
if (portal->status == PORTAL_ACTIVE)
portal->status = PORTAL_FAILED;
! /*
! * If the portal is READY then allow it to survive into the parent
! * transaction; otherwise shut it down.
! */
! if (portal->status == PORTAL_READY)
! {
! portal->createSubid = parentSubid;
! if (portal->resowner)
! ResourceOwnerNewParent(portal->resowner, parentXactOwner);
! }
! else
{
! /* let portalcmds.c clean up the state it knows about */
! if (PointerIsValid(portal->cleanup))
! {
! (*portal->cleanup) (portal);
! portal->cleanup = NULL;
! }
!
! /*
! * Any resources belonging to the portal will be released in
! * the upcoming transaction-wide cleanup; they will be gone
! * before we run PortalDrop.
! */
! portal->resowner = NULL;
}
}
}
--- 631,655 ----
* will go FAILED if the underlying cursor fails. (Note we do NOT
* want to do this to upper-level portals, since they may be able
* to continue.)
+ *
+ * This is only needed to dodge the sanity check in PortalDrop.
*/
if (portal->status == PORTAL_ACTIVE)
portal->status = PORTAL_FAILED;
! /* let portalcmds.c clean up the state it knows about */
! if (PointerIsValid(portal->cleanup))
{
! (*portal->cleanup) (portal);
! portal->cleanup = NULL;
}
+
+ /*
+ * Any resources belonging to the portal will be released in
+ * the upcoming transaction-wide cleanup; they will be gone
+ * before we run PortalDrop.
+ */
+ portal->resowner = NULL;
}
}
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
On Wed, Jan 26, 2005 at 05:10:09PM -0500, Tom Lane wrote:
I don't think we have a lot of choices: we have to destroy (or at least
mark FAILED) all such cursors for the time being.
I don't see a lot of difference between marking the portal FAILED and
destroying it (maybe I'm looking at the wrong code). So I just took the
simpler approach; patch attached.
Mainly just that it'll be easier to improve the code later if we don't
have to put back something we removed. I'll tweak the patch to do that.
regards, tom lane
On Wed, 2005-01-26 at 12:02 -0300, Alvaro Herrera wrote:
The policy will now be: cursor creation is transaction, but cursor state
modifications (FETCH) are non-transactional -- right? I wonder if it
wouldn't be more consistent to make cursor deletion (CLOSE)
transactional as well -- so that a CLOSE in an aborted subtransaction
would not actually destroy the cursor.Hmm ... not sure how hard that is.
Would it work to record the sub XID of the deleting subtxn on CLOSE, and
then consider whether to "really" do the deletion when the subtxn
commits/aborts?
-Neil
Neil Conway <neilc@samurai.com> writes:
On Wed, 2005-01-26 at 12:02 -0300, Alvaro Herrera wrote:
Hmm ... not sure how hard that is.
Would it work to record the sub XID of the deleting subtxn on CLOSE, and
then consider whether to "really" do the deletion when the subtxn
commits/aborts?
It'd take more than that. Consider
BEGIN;
DECLARE c CURSOR ...;
SAVEPOINT x;
CLOSE c;
DECLARE c CURSOR ...; -- draws duplicate-cursor error
You'd need to do something to "hide" the deleted cursor for the
remainder of its subtransaction.
regards, tom lane
On Wed, 2005-01-26 at 12:02 -0300, Alvaro Herrera wrote:
and some regression tests for this behavior, but I'm happy to add that
myself if no one beats me to it.Please do.
Attached is a patch adding regression tests for this change -- I've
already applied it to HEAD.
-Neil
Attachments:
cursor_savepoint_tests-1.patchtext/x-patch; charset=ISO-8859-1; name=cursor_savepoint_tests-1.patchDownload
Index: src/test/regress/expected/transactions.out
===================================================================
RCS file: /var/lib/cvs/pgsql/src/test/regress/expected/transactions.out,v
retrieving revision 1.10
diff -c -r1.10 transactions.out
*** src/test/regress/expected/transactions.out 13 Sep 2004 20:09:51 -0000 1.10
--- src/test/regress/expected/transactions.out 27 Jan 2005 01:25:43 -0000
***************
*** 470,472 ****
--- 470,519 ----
DROP TABLE foo;
DROP TABLE baz;
DROP TABLE barbaz;
+ -- verify that cursors created during an aborted subtransaction are
+ -- closed, but that we do not rollback the effect of any FETCHs
+ -- performed in the aborted subtransaction
+ begin;
+ savepoint x;
+ create table abc (a int);
+ insert into abc values (5);
+ insert into abc values (10);
+ declare foo cursor for select * from abc;
+ fetch from foo;
+ a
+ ---
+ 5
+ (1 row)
+
+ rollback to x;
+ -- should fail
+ fetch from foo;
+ ERROR: cursor "foo" does not exist
+ commit;
+ begin;
+ create table abc (a int);
+ insert into abc values (5);
+ insert into abc values (10);
+ insert into abc values (15);
+ declare foo cursor for select * from abc;
+ fetch from foo;
+ a
+ ---
+ 5
+ (1 row)
+
+ savepoint x;
+ fetch from foo;
+ a
+ ----
+ 10
+ (1 row)
+
+ rollback to x;
+ fetch from foo;
+ a
+ ----
+ 15
+ (1 row)
+
+ abort;
Index: src/test/regress/sql/transactions.sql
===================================================================
RCS file: /var/lib/cvs/pgsql/src/test/regress/sql/transactions.sql,v
retrieving revision 1.10
diff -c -r1.10 transactions.sql
*** src/test/regress/sql/transactions.sql 13 Sep 2004 20:10:13 -0000 1.10
--- src/test/regress/sql/transactions.sql 27 Jan 2005 01:23:37 -0000
***************
*** 290,292 ****
--- 290,327 ----
DROP TABLE foo;
DROP TABLE baz;
DROP TABLE barbaz;
+
+ -- verify that cursors created during an aborted subtransaction are
+ -- closed, but that we do not rollback the effect of any FETCHs
+ -- performed in the aborted subtransaction
+ begin;
+
+ savepoint x;
+ create table abc (a int);
+ insert into abc values (5);
+ insert into abc values (10);
+ declare foo cursor for select * from abc;
+ fetch from foo;
+ rollback to x;
+
+ -- should fail
+ fetch from foo;
+ commit;
+
+ begin;
+
+ create table abc (a int);
+ insert into abc values (5);
+ insert into abc values (10);
+ insert into abc values (15);
+ declare foo cursor for select * from abc;
+
+ fetch from foo;
+
+ savepoint x;
+ fetch from foo;
+ rollback to x;
+
+ fetch from foo;
+
+ abort;
Alvaro Herrera wrote:
On Wed, Jan 26, 2005 at 05:10:09PM -0500, Tom Lane wrote:
I don't think we have a lot of choices: we have to destroy (or at least
mark FAILED) all such cursors for the time being.I don't see a lot of difference between marking the portal FAILED and
destroying it (maybe I'm looking at the wrong code). So I just took the
simpler approach; patch attached.
I assume that you can CLOSE a failed portal, but you can't CLOSE a
destroyed portal (because it's not there any more)?
This is important for the JDBC driver as it creates portals internally,
does fetches as the application code demands, then closes the portal at
some point after the application is done with it. Having the close fail
because of an intervening savepoint rollback isn't great -- the error
will cause an unexpected failure of the current transaction. This can
happen even if the application doesn't try to use the portal (via
ResultSet) after the savepoint rollback at all.
It wouldn't be so bad if the driver could track savepoint boundaries,
but the current protocol doesn't make that easy..
-O
Oliver Jowett wrote:
Having the close fail
because of an intervening savepoint rollback isn't great -- the error
will cause an unexpected failure of the current transaction.
Never mind -- I just reread the protocol docs, and it's safe to close a
nonexistant portal. Did this previously issue a warning, or something
similar? I'm sure I had seen problems in this area in the past..
-O
On Fri, Jan 28, 2005 at 12:27:05PM +1300, Oliver Jowett wrote:
Oliver Jowett wrote:
Having the close fail
because of an intervening savepoint rollback isn't great -- the error
will cause an unexpected failure of the current transaction.Never mind -- I just reread the protocol docs, and it's safe to close a
nonexistant portal. Did this previously issue a warning, or something
similar? I'm sure I had seen problems in this area in the past..
Not sure ... however I remember somebody complained because the SQL
level interface differed from the protocol level one on this point.
--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"In a specialized industrial society, it would be a disaster
to have kids running around loose." (Paul Graham)