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

My comments were in no way meant as an argument for or against the
change itself. Only to clearly document the side effect it will have.

Regards, Jan

On 11/7/22 11:57, Kirk Wolak wrote:
>
>
> On Mon, Nov 7, 2022 at 11:10 AM Jan Wieck <jan(at)wi3ck(dot)info
> <mailto:jan(at)wi3ck(dot)info>> wrote:
>
> On 11/4/22 19:46, Tom Lane wrote:
> > Jan Wieck <jan(at)wi3ck(dot)info <mailto: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!
>
>