Re: PL/pgSQL cursors should get generated portal names by default

On Mon, Nov 7, 2022 at 11:10 AM Jan Wieck <jan(at)wi3ck(dot)info> wrote:

> On 11/4/22 19:46, Tom Lane wrote:
> > Jan Wieck <jan(at)wi3ck(dot)info> writes:
> >> I need to do some testing on this. I seem to recall that the naming was
> >> originally done because a reference cursor is basically a named cursor
> >> that can be handed around between functions and even the top SQL level
> >> of the application. For the latter to work the application needs to
> know
> >> the name of the portal.
> >
> > Right. With this patch, it'd be necessary to hand back the actual
> > portal name (by returning the refcursor value), or else manually
> > set the refcursor value before OPEN to preserve the previous behavior.
> > But as far as I saw, all our documentation examples show handing back
> > the portal name, so I'm hoping most people do it like that already.
>
> I was mostly concerned that we may unintentionally break underdocumented
> behavior that was originally implemented on purpose. As long as everyone
> is aware that this is breaking backwards compatibility in the way it
> does, that's fine.
>

I respect the concern, and applied some deeper thinking to it...

Here is the logic I am applying to this compatibility issue and what may
break.
[FWIW, my motto is to be wrong out loud, as you learn faster]

At first pass, I thought "Well, since this does not break a refcursor,
which is the obvious use case for RETURNING/PASSING, we are fine!"

But in trying to DEFEND this case, I have come up with example of code
(that makes some SENSE, but would break):

CREATE FUNCTION test() RETURNS refcursor() LANGUAGE plpgsql AS $$
DECLARE
cur_this cursor FOR SELECT 1;
ref_cur refcursor;
BEGIN
OPEN cur_this;
ref_cur := 'cur_this'; -- Using the NAME of the cursor as the portal
name: Should do: ref_cur := cur_this; -- Only works after OPEN
RETURN ref_cur;
END;
$$;

As noted in the comments. If the code were:
ref_cur := 'cur_this'; -- Now you can't just use ref_cur := cur_this;
OPEN cur_this;
RETURN ref_cur;
Then it would break now... And even the CORRECT syntax would break, since
the cursor was not opened, so "cur_this" is null.

Now, I have NO IDEA if someone would actually do this. It is almost
pathological. The use case would be a complex cursor with parameters,
and they changed the code to return a refcursor!
This was the ONLY use case I could think of that wasn't HACKY!

HACKY use cases involve a child routine setting: local_ref_cursor :=
'cur_this'; in order to access a cursor that was NOT passed to the child.
FWIW, I tested this, and it works, and I can FETCH in the child routine,
and it affects the parents' LOOP as it should... WOW. I would be HAPPY
to break such horrible code, it has to be a security concern at some level.

Personally (and my 2 cents really shouldn't matter much), I think this
should still be fixed.
Because I believe this small use case is rare, it will break immediately,
and the fix is trivial (just initialize cur_this := 'cur_this' in this
example),
and the fix removes the Orthogonal Behavior Tom pointed out, which led me
to reporting this.

I think I have exhausted examples of how this impacts a VALID
refcursor implementation. I believe any other such versions are variations
of this!
And maybe we document that if a refcursor of a cursor is to be returned,
that the refcursor is ASSIGNED after the OPEN of the cursor, and it is done
without the quotes, as:
ref_cursor := cur_this; -- assign the name after opening.

Thanks!