Обсуждение: Bug in PL/pgSQL FOR cursor variant
postgres=# CREATE FUNCTION func() RETURNS VOID AS $$ declare cur CURSOR IS SELECT generate_series(1,10) AS a; BEGIN FOR erec IN cur LOOP raise notice 'row %', erec.a ; IF (erec.a = 5) THEN CLOSE cur; END IF; END LOOP; RETURN; END; $$ LANGUAGE plpgsql; CREATE FUNCTION postgres=# SELECT func(); NOTICE: row 1 NOTICE: row 2 NOTICE: row 3 NOTICE: row 4 NOTICE: row 5 server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. Reproducible on 8.4 and CVS HEAD, the "FOR cursor" statement didn't exist on earlier versions. The problem is that exec_stmt_forc keeps using a pointer to the Portal, which becomes invalid if the cursor is closed in the middle. Patch attached, will apply.. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Вложения
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > The problem is that exec_stmt_forc keeps using a pointer to the Portal, > which becomes invalid if the cursor is closed in the middle. Patch > attached, will apply.. Does that really fix anything? I suspect you need to pstrdup() the portalname. Also, isn't exec_for_query() at just as much risk? The latter's problem would only be exposed if the cursor was closed at a batch boundary, but it's still a problem. I wonder whether we ought to try to make it an error to close a portal that's still in use. regards, tom lane
On 21/06/10 17:08, Tom Lane wrote: > I suspect you need to pstrdup() the portalname. Yes, you're right. Thanks. > Also, isn't exec_for_query() at just as much risk? > The latter's problem would only be exposed if the cursor was closed > at a batch boundary, but it's still a problem. Can you elaborate? I thought I fixed exec_for_query(). (except for the missing pstrdup). > I wonder whether we ought to try to make it an error to close a portal > that's still in use. I think it's fine as it is. FWIW what we do now matches Oracle. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > On 21/06/10 17:08, Tom Lane wrote: >> Also, isn't exec_for_query() at just as much risk? >> The latter's problem would only be exposed if the cursor was closed >> at a batch boundary, but it's still a problem. > Can you elaborate? I thought I fixed exec_for_query(). (except for the > missing pstrdup). Oh, I thought I'd read the whole patch, but I see I missed the last part. But it doesn't matter, because that's still broken, both as to performance and security. prefetch_ok is not meant to be bulletproof, only to ensure that in cases where the cursor is *meant* to be exposed to the user its behavior is as he expects. If you're trying to stop a crash you need to realize that people can get at any portal at all. On the performance end, redoing SPI_cursor_find every row seems like rather a large penalty ... regards, tom lane
On 21/06/10 22:25, Tom Lane wrote: > prefetch_ok is not meant to be bulletproof, > only to ensure that in cases where the cursor is *meant* to be exposed > to the user its behavior is as he expects. If you're trying to stop a > crash you need to realize that people can get at any portal at all. Oh, I see. I didn't realize that unnamed cursors are still accessible to anyone with the "<unnamed portal X>" name. > On the performance end, redoing SPI_cursor_find every row seems like > rather a large penalty ... Granted. Maybe it would be easier to somehow protect the portal then, and throw an error if you try to close it. We could just mark the portal as PORTAL_ACTIVE while we run the user statements, but that would also forbid fetching or moving it. I'm thinking of a new "pinned" state, which is like PORTAL_READY except that the portal can't be dropped like in PORTAL_ACTIVE state. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Excerpts from Heikki Linnakangas's message of lun jun 21 17:47:43 -0400 2010: > Maybe it would be easier to somehow protect the portal then, and throw > an error if you try to close it. We could just mark the portal as > PORTAL_ACTIVE while we run the user statements, but that would also > forbid fetching or moving it. I'm thinking of a new "pinned" state, > which is like PORTAL_READY except that the portal can't be dropped like > in PORTAL_ACTIVE state. Why is it an error to close the portal? Maybe we should keep it closed (i.e. don't free it), and error out only when it is accessed again. -- Ãlvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 22/06/10 00:59, Alvaro Herrera wrote: > Excerpts from Heikki Linnakangas's message of lun jun 21 17:47:43 -0400 2010: > >> Maybe it would be easier to somehow protect the portal then, and throw >> an error if you try to close it. We could just mark the portal as >> PORTAL_ACTIVE while we run the user statements, but that would also >> forbid fetching or moving it. I'm thinking of a new "pinned" state, >> which is like PORTAL_READY except that the portal can't be dropped like >> in PORTAL_ACTIVE state. > > Why is it an error to close the portal? What useful behavior áºould you expect from closing it? > Maybe we should keep it closed > (i.e. don't free it), and error out only when it is accessed again. I guess we could do that too, but it really doesn't make much sense to close it in the first place. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 22/06/10 00:47, Heikki Linnakangas wrote: > Maybe it would be easier to somehow protect the portal then, and throw > an error if you try to close it. We could just mark the portal as > PORTAL_ACTIVE while we run the user statements, but that would also > forbid fetching or moving it. I'm thinking of a new "pinned" state, > which is like PORTAL_READY except that the portal can't be dropped like > in PORTAL_ACTIVE state. Like this. (I'll need to revert the broken commit before applying this) -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Вложения
Excerpts from Heikki Linnakangas's message of lun jun 21 18:29:48 -0400 2010: > On 22/06/10 00:59, Alvaro Herrera wrote: > > Excerpts from Heikki Linnakangas's message of lun jun 21 17:47:43 -0400 2010: > > > >> Maybe it would be easier to somehow protect the portal then, and throw > >> an error if you try to close it. We could just mark the portal as > >> PORTAL_ACTIVE while we run the user statements, but that would also > >> forbid fetching or moving it. I'm thinking of a new "pinned" state, > >> which is like PORTAL_READY except that the portal can't be dropped like > >> in PORTAL_ACTIVE state. > > > > Why is it an error to close the portal? > > What useful behavior áºould you expect from closing it? I don't know; it was you who said that the current behavior mimicked Oracle. If it's not useful, then I don't have a problem with your proposal. -- Ãlvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 22/06/10 01:49, Heikki Linnakangas wrote: > On 22/06/10 00:47, Heikki Linnakangas wrote: >> Maybe it would be easier to somehow protect the portal then, and throw >> an error if you try to close it. We could just mark the portal as >> PORTAL_ACTIVE while we run the user statements, but that would also >> forbid fetching or moving it. I'm thinking of a new "pinned" state, >> which is like PORTAL_READY except that the portal can't be dropped like >> in PORTAL_ACTIVE state. > > Like this. > > (I'll need to revert the broken commit before applying this) Ok, here's my final patch for this that I'll commit shortly. I added a new boolean for the "pinned" status in the end, rather than confuse it with the portal status. It's simpler that way. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com