Thread: BUG #18859: ERROR: unexpected plan node type: 356

The following bug has been logged on the website:

Bug reference:      18859
Logged by:          Olleg Samoylov
Email address:      splarv@ya.ru
PostgreSQL version: 17.4
Operating system:   RedOS 8
Description:

This working.
postgres=> do $$ declare p_CurData refcursor; begin OPEN p_CurData FOR
SELECT NULL::int id; end;$$;
DO
But this is not. Internal error.
postgres=> do $$ declare p_CurData refcursor; begin OPEN p_CurData SCROLL
FOR SELECT NULL::int id; end;$$;
ERROR:  unexpected plan node type: 356
CONTEXT:  PL/pgSQL function inline_code_block line 1 at OPEN

On 20/3/2025 14:53, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      18859
> Logged by:          Olleg Samoylov
> Email address:      splarv@ya.ru
> PostgreSQL version: 17.4
> Operating system:   RedOS 8
> Description:
> 
> This working.
> postgres=> do $$ declare p_CurData refcursor; begin OPEN p_CurData FOR
> SELECT NULL::int id; end;$$;
> DO
> But this is not. Internal error.
> postgres=> do $$ declare p_CurData refcursor; begin OPEN p_CurData SCROLL
> FOR SELECT NULL::int id; end;$$;
> ERROR:  unexpected plan node type: 356
> CONTEXT:  PL/pgSQL function inline_code_block line 1 at OPEN
> 
It may be reproduced with any expression, treated as a simple 
expression. For example:

DO $$
DECLARE
  p_CurData refcursor;
BEGIN
  OPEN p_CurData SCROLL FOR SELECT now();
END; $$;

The problem here is in the scrollable cursors code which inserts 
Material node:

    if (cursorOptions & CURSOR_OPT_SCROLL)
    {
        if (!ExecSupportsBackwardScan(top_plan))
            top_plan = materialize_finished_plan(top_plan);
    }

But the exec_save_simple_expr() doesn't process Material node and causes 
the ERROR.

-- 
regards, Andrei Lepikhov

Andrei Lepikhov <lepihov@gmail.com> writes:
> The problem here is in the scrollable cursors code which inserts 
> Material node:

Yeah, I'd just come to the same conclusion.  I guess we can make
this code look through a Material node as well as Gather.

            regards, tom lane

On 21/3/2025 14:00, Tom Lane wrote:
> Andrei Lepikhov <lepihov@gmail.com> writes:
>> The problem here is in the scrollable cursors code which inserts
>> Material node:
> 
> Yeah, I'd just come to the same conclusion.  I guess we can make
> this code look through a Material node as well as Gather.
Yes, as I see there are no additional corner cases. See the code in 
attachment.

-- 
regards, Andrei Lepikhov

On Fri, Mar 21, 2025 at 11:49 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 21/3/2025 14:00, Tom Lane wrote:
> > Andrei Lepikhov <lepihov@gmail.com> writes:
> >> The problem here is in the scrollable cursors code which inserts
> >> Material node:

> > Yeah, I'd just come to the same conclusion.  I guess we can make
> > this code look through a Material node as well as Gather.

> Yes, as I see there are no additional corner cases. See the code in
> attachment.

A Material's tlist could also be a Const copied up by setrefs.c, in
which case we can avoid looking further, similar to what Gather does.
I wonder if we could have Material use the same handle as Gather.

-       else if (IsA(plan, Gather))
+       else if (IsA(plan, Gather) || IsA(plan, Material))

Thanks
Richard

Andrei Lepikhov <lepihov@gmail.com> writes:
> On 21/3/2025 14:00, Tom Lane wrote:
>> Yeah, I'd just come to the same conclusion.  I guess we can make
>> this code look through a Material node as well as Gather.

> Yes, as I see there are no additional corner cases. See the code in
> attachment.

I think actually we want to use the same processing as for Gather,
in particular the check for a Const.  That's all about what
setrefs.c will do to the plan tree, and it'll do the same things
to Material as it would to Gather.  Since that stanza doesn't
actually do anything that's specific to Gather, the code change
can be as simple as

@@ -8323,7 +8325,7 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
                    ((Result *) plan)->resconstantqual == NULL);
             break;
         }
-        else if (IsA(plan, Gather))
+        else if (IsA(plan, Gather) || IsA(plan, Material))
         {
             Assert(plan->lefttree != NULL &&
                    plan->righttree == NULL &&

I fixed that, did some cosmetic fooling with the comment
and test case, and pushed it.  Thanks for the report,
and for the patch!

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> A Material's tlist could also be a Const copied up by setrefs.c, in
> which case we can avoid looking further, similar to what Gather does.
> I wonder if we could have Material use the same handle as Gather.

Right, I'd come to the same conclusion before seeing your message.

            regards, tom lane