Re: [HACKERS] bug w/ cursors and savepoints
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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] bug w/ cursors and savepoints
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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] bug w/ cursors and savepoints
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 ([EMAIL PROTECTED]) In a specialized industrial society, it would be a disaster to have kids running around loose. (Paul Graham) ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] bug w/ cursors and savepoints
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 ([EMAIL PROTECTED]) La espina, desde que nace, ya pincha (Proverbio africano) ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] bug w/ cursors and savepoints
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 ([EMAIL PROTECTED]) Ciencias políticas es la ciencia de entender por qué los políticos actúan como lo hacen (netfunny.com) ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] bug w/ cursors and savepoints
Alvaro Herrera [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] bug w/ cursors and savepoints
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] bug w/ cursors and savepoints
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 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 - 1.10 --- src/test/regress/expected/transactions.out 27 Jan 2005 01:25:43 - *** *** 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 - 1.10 --- src/test/regress/sql/transactions.sql 27 Jan 2005 01:23:37 - *** *** 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; ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] bug w/ cursors and savepoints
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] -- 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] bug w/ cursors and savepoints
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] bug w/ cursors and savepoints
On Tue, Jan 25, 2005 at 02:40:51AM -0500, Tom Lane wrote: Neil Conway [EMAIL PROTECTED] 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 ([EMAIL PROTECTED]) La rebeldía es la virtud original del hombre (Arthur Schopenhauer) ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] bug w/ cursors and savepoints
Alvaro Herrera [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] bug w/ cursors and savepoints
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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] bug w/ cursors and savepoints
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 ([EMAIL PROTECTED])
The ability to monopolize a planet is insignificant
next to the power of the source
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 - 1.76
--- portalmem.c 25 Jan 2005 17:52:52 -
***
*** 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;
}
}
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] bug w/ cursors and savepoints
Alvaro Herrera [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] bug w/ cursors and savepoints
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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] bug w/ cursors and savepoints
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
