Re: 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(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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