CALL stmt, ERROR: unrecognized node type: 113 bug

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

CALL stmt, ERROR: unrecognized node type: 113 bug

Pavel Stehule
Hi

I am playing with procedures little bit

I found few bugs

create procedure test(a int)
as $$
begin
  raise notice '>>>%<<<', a;
end;
$$ language plpgsql;

call test(10); -- ok

postgres=# call test((select 10));
ERROR:  unrecognized node type: 113

postgres=# \sf test
ERROR:  cache lookup failed for type 0

Regards

Pavel

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier
On Thu, Feb 01, 2018 at 05:33:54PM +0100, Pavel Stehule wrote:

> I am playing with procedures little bit
>
> I found few bugs
>
> create procedure test(a int)
> as $$
> begin
>   raise notice '>>>%<<<', a;
> end;
> $$ language plpgsql;
>
> call test(10); -- ok
>
> postgres=# call test((select 10));
> ERROR:  unrecognized node type: 113
>
> postgres=# \sf test
> ERROR:  cache lookup failed for type 0
Peter, Andrew, this is missing some bits related to the conversion of
SubLink nodes to SubPlan nodes for procedures when used as argument of
a procedure as only the latter can be executed after the former is
processed by the latter (see SS_process_sublinks).
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier
On Fri, Feb 02, 2018 at 04:01:13PM +0900, Michael Paquier wrote:
> Peter, Andrew, this is missing some bits related to the conversion of
> SubLink nodes to SubPlan nodes for procedures when used as argument of
> a procedure as only the latter can be executed after the former is
> processed by the latter (see SS_process_sublinks).

Meh-to-self.

You need to read that as "only a SubPlan can be executed after a SubLink
has been processed by the planner", so please replace the last "latter"
by "planner".
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier-2
On Fri, Feb 02, 2018 at 04:07:28PM +0900, Michael Paquier wrote:
> You need to read that as "only a SubPlan can be executed after a SubLink
> has been processed by the planner", so please replace the last "latter"
> by "planner".

(I forgot to add Peter and Andrew in CC: previously, so done now.)

e4128ee7 is making is clear that SubLink are authorized when
transforming it in transformSubLink(), however I cannot think about a
use case so should we just forbid them, and this is actually untested.
So the patch attached does so.

The second problem involves a cache lookup failure for a type when
trying to use pg_get_functiondef on a procedure.  Luckily, it is
possible to make the difference between a procedure and a function by
checking if prorettype is InvalidOid or not.  There is room for a new
patch which supports pg_get_proceduredef() to generate the definition of
a procedure, with perhaps a dedicated psql shortcut, but that could
always be done later on.
--
Michael

0001-Fix-minor-issues-with-procedure-calls.patch (4K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Pavel Stehule
Hi

2018-02-09 7:56 GMT+01:00 Michael Paquier <[hidden email]>:
On Fri, Feb 02, 2018 at 04:07:28PM +0900, Michael Paquier wrote:
> You need to read that as "only a SubPlan can be executed after a SubLink
> has been processed by the planner", so please replace the last "latter"
> by "planner".

(I forgot to add Peter and Andrew in CC: previously, so done now.)

e4128ee7 is making is clear that SubLink are authorized when
transforming it in transformSubLink(), however I cannot think about a
use case so should we just forbid them, and this is actually untested.
So the patch attached does so.

The second problem involves a cache lookup failure for a type when
trying to use pg_get_functiondef on a procedure.  Luckily, it is
possible to make the difference between a procedure and a function by
checking if prorettype is InvalidOid or not.  There is room for a new
patch which supports pg_get_proceduredef() to generate the definition of
a procedure, with perhaps a dedicated psql shortcut, but that could
always be done later on.

Blocking subqueries in CALL parameters is possible solution. But blocking func def for procedures without any substitution doesn't look correct for me.

Regards

Pavel
 
--
Michael

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier-2
On Fri, Feb 09, 2018 at 12:02:57PM +0100, Pavel Stehule wrote:

> 2018-02-09 7:56 GMT+01:00 Michael Paquier <[hidden email]>:
> > The second problem involves a cache lookup failure for a type when
> > trying to use pg_get_functiondef on a procedure.  Luckily, it is
> > possible to make the difference between a procedure and a function by
> > checking if prorettype is InvalidOid or not.  There is room for a new
> > patch which supports pg_get_proceduredef() to generate the definition of
> > a procedure, with perhaps a dedicated psql shortcut, but that could
> > always be done later on.
>
> Blocking subqueries in CALL parameters is possible solution.
ExecuteCallStmt has visibly been written so as it is able to handle the
input set of arguments with a minimal infrastructure in place.  SubLink
nodes require more advanced handling as those need to go through the
planner.  There is also additional processing in the rewriter.  At the
end I tend to think that any user would just turn their back on calling
a function for such cases anyway, so it seems to me that the potential
benefits are not worth the code complexity.

> But blocking
> func def for procedures without any substitution doesn't look correct for
> me.

I don't disagree with you here, there is room for such a function, but
on the other side not having it does not make the existing feature less
useful.  As it is Peter's and Andrew's feature, the last word should
come from them.  Here is my opinion for what it's worth:
- Procedures are not functions, the code is pretty clear about that, so
a system function to generate the definition of a procedure should not
happen with pg_get_functiondef().  They share a lot of infrastructure so
you can reuse a lot of the code present.
- A different psql shortcut should be used, and I would assume that \sp
is adapted.
- Aggregates are also in pg_proc, we generate an error on them because
they are of different type, so an error for procedures when trying to
fetch a functoin definition looks like the good answer.

If folks feel that having a way to retrieve the procedure definition
easily via ruleutils.c is a must-have, then we have material for a good
open item :)
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

David G Johnston
On Fri, Feb 9, 2018 at 6:23 AM, Michael Paquier <[hidden email]> wrote:
On Fri, Feb 09, 2018 at 12:02:57PM +0100, Pavel Stehule wrote:
> 2018-02-09 7:56 GMT+01:00 Michael Paquier <[hidden email]>:
> > The second problem involves a cache lookup failure for a type when
> > trying to use pg_get_functiondef on a procedure.  Luckily, it is
> > possible to make the difference between a procedure and a function by
> > checking if prorettype is InvalidOid or not.  There is room for a new
> > patch which supports pg_get_proceduredef() to generate the definition of
> > a procedure, with perhaps a dedicated psql shortcut, but that could
> > always be done later on.
>
> Blocking subqueries in CALL parameters is possible solution.

ExecuteCallStmt has visibly been written so as it is able to handle the
input set of arguments with a minimal infrastructure in place.  SubLink
nodes require more advanced handling as those need to go through the
planner.  There is also additional processing in the rewriter.  At the
end I tend to think that any user would just turn their back on calling
a function for such cases anyway, so it seems to me that the potential
benefits are not worth the code complexity.

​CALL is not just a different syntax for function invocation - if you want the properties of CALL then falling back to SELECT function() is not a valid alternative.​

To me this feels like an interaction between two features that users are going to expect to just work.  Current discussions lead me to think that is something we strive to provide unless a strong argument against is provided.  I'm not sure added code complexity here is going to make the grade even if I cannot reasonably judge just how much complexity is involved.

David J.

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Pavel Stehule


2018-02-09 15:15 GMT+01:00 David G. Johnston <[hidden email]>:
On Fri, Feb 9, 2018 at 6:23 AM, Michael Paquier <[hidden email]> wrote:
On Fri, Feb 09, 2018 at 12:02:57PM +0100, Pavel Stehule wrote:
> 2018-02-09 7:56 GMT+01:00 Michael Paquier <[hidden email]>:
> > The second problem involves a cache lookup failure for a type when
> > trying to use pg_get_functiondef on a procedure.  Luckily, it is
> > possible to make the difference between a procedure and a function by
> > checking if prorettype is InvalidOid or not.  There is room for a new
> > patch which supports pg_get_proceduredef() to generate the definition of
> > a procedure, with perhaps a dedicated psql shortcut, but that could
> > always be done later on.
>
> Blocking subqueries in CALL parameters is possible solution.

ExecuteCallStmt has visibly been written so as it is able to handle the
input set of arguments with a minimal infrastructure in place.  SubLink
nodes require more advanced handling as those need to go through the
planner.  There is also additional processing in the rewriter.  At the
end I tend to think that any user would just turn their back on calling
a function for such cases anyway, so it seems to me that the potential
benefits are not worth the code complexity.

​CALL is not just a different syntax for function invocation - if you want the properties of CALL then falling back to SELECT function() is not a valid alternative.​

+1
 

To me this feels like an interaction between two features that users are going to expect to just work.  Current discussions lead me to think that is something we strive to provide unless a strong argument against is provided.  I'm not sure added code complexity here is going to make the grade even if I cannot reasonably judge just how much complexity is involved.

when some procedure can do transaction control, or can returns possible set or multirecord set (in future), then 100% agree, so it is different creature then function. But if not, then it should be specified why it is different from void function.

I don't understand, why we should to prohibit subqueries as procedure params - with some limits. I can understand to requirement to not change any data.

Regards

Pavel


 

David J.


Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Tom Lane-2
In reply to this post by David G Johnston
"David G. Johnston" <[hidden email]> writes:
> On Fri, Feb 9, 2018 at 6:23 AM, Michael Paquier <[hidden email]> wrote:
>> On Fri, Feb 09, 2018 at 12:02:57PM +0100, Pavel Stehule wrote:
>>> Blocking subqueries in CALL parameters is possible solution.

> To me this feels like an interaction between two features that users are
> going to expect to just work.

Meh.  It doesn't look significantly different to me than the restriction
that you can't have sub-selects in CHECK expressions, index expressions,
etc.  Obviously we need a clean failure like you get for those cases.
But otherwise it's an OK restriction that stems from exactly the same
cause: we do not want to invoke the full planner in this context (and
even if we did, we don't want to use the full executor to execute the
result).

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

David G Johnston
On Fri, Feb 9, 2018 at 7:42 AM, Tom Lane <[hidden email]> wrote:
"David G. Johnston" <[hidden email]> writes:
> On Fri, Feb 9, 2018 at 6:23 AM, Michael Paquier <[hidden email]> wrote:
>> On Fri, Feb 09, 2018 at 12:02:57PM +0100, Pavel Stehule wrote:
>>> Blocking subqueries in CALL parameters is possible solution.

> To me this feels like an interaction between two features that users are
> going to expect to just work.

Meh.  It doesn't look significantly different to me than the restriction
that you can't have sub-selects in CHECK expressions, index expressions,
etc.  Obviously we need a clean failure like you get for those cases.
But otherwise it's an OK restriction that stems from exactly the same
cause: we do not want to invoke the full planner in this context (and
even if we did, we don't want to use the full executor to execute the
result).

Does/Should:

CALL test(func(10)); --with or without an extra set of parentheses

work here too?

David J.

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Andres Freund
In reply to this post by Tom Lane-2
On 2018-02-09 09:42:41 -0500, Tom Lane wrote:
> It doesn't look significantly different to me than the restriction
> that you can't have sub-selects in CHECK expressions, index
> expressions, etc.  Obviously we need a clean failure like you get for
> those cases.  But otherwise it's an OK restriction that stems from
> exactly the same cause: we do not want to invoke the full planner in
> this context (and even if we did, we don't want to use the full
> executor to execute the result).

+1

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier-2
On Fri, Feb 09, 2018 at 08:30:49AM -0800, Andres Freund wrote:

> On 2018-02-09 09:42:41 -0500, Tom Lane wrote:
>> It doesn't look significantly different to me than the restriction
>> that you can't have sub-selects in CHECK expressions, index
>> expressions, etc.  Obviously we need a clean failure like you get for
>> those cases.  But otherwise it's an OK restriction that stems from
>> exactly the same cause: we do not want to invoke the full planner in
>> this context (and even if we did, we don't want to use the full
>> executor to execute the result).
>
> +1
+1.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Fri, Feb 09, 2018 at 08:30:49AM -0800, Andres Freund wrote:
>> On 2018-02-09 09:42:41 -0500, Tom Lane wrote:
>>> ...  But otherwise it's an OK restriction that stems from
>>> exactly the same cause: we do not want to invoke the full planner in
>>> this context (and even if we did, we don't want to use the full
>>> executor to execute the result).

So I idly looked at ExecuteCallStmt to see how, in fact, it is executing
these things, and I was right to guess that it couldn't possibly support
a sub-select, since it's just using ExecEvalExprSwitchContext.  So we
need to go teach transformSubLink that EXPR_KIND_CALL is *not* okay,
which is simple enough.

However, I also wondered how ExecuteCallStmt works at all for pass-by-
reference datatypes, since it immediately destroys the execution context
for each expression.  And the answer is that it doesn't, as proven here:

regression=# create procedure myp(f1 text) as $$begin
regression$# raise notice 'f1 = %', f1;
regression$# end$$ language plpgsql;
CREATE PROCEDURE
regression=# call myp('xyzzy');
NOTICE:  f1 = xyzzy
CALL
regression=# call myp('xyzzy' || 'x');
NOTICE:  f1 =
CALL

The call with a literal constant accidentally seems to work, because the
returned Datum is actually pointing into the original expression tree.
But if you have the expression do any actual work, then not so much.

I think this could be fixed by evaluating all the arguments in a single
execution context that is not destroyed till after the call finishes
(using a separate one for each argument seems pretty silly anyway).
However, the code could do with more than zero commentary about how come
this is safe at all --- even if we keep the execution context, it's still
a child of whatever random memory context ExecuteCallStmt was called in,
and it's not very clear that that context will survive a transaction
commit.

This does not leave me with a warm feeling about either the amount
of testing the procedure feature has gotten, or the state of its
internal documentation.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Tom Lane-2
I wrote:
> However, I also wondered how ExecuteCallStmt works at all for pass-by-
> reference datatypes, since it immediately destroys the execution context
> for each expression.  And the answer is that it doesn't, as proven here:

On closer inspection, there are actually three sub-cases involved.
It accidentally works for simple constant arguments, because the
passed Datum will point at a Const node generated during transformExpr.
And it works for fully run-time-evaluated arguments, because those end
up in memory belonging to the standalone ExprContext(s), which
ExecuteCallStmt never bothers to free at all.  (Which is a bug in itself,
although possibly one that wouldn't be exposed in practice given that
we disallow SRFs here; I don't know if there are any other cases that
would expect ExprContext cleanup hooks to get invoked.)  Where it doesn't
work is for expressions that are const-folded during ExecPrepareExpr,
because then the values are in Const nodes that live in the EState's
per-query context, and the code is throwing that away too soon.

I pushed a fix for all that.

The failure in pg_get_functiondef() is still there.  While the immediate
answer probably is to teach that function to emit correct CREATE PROCEDURE
syntax, I continue to think that it's a bad idea to be putting zeroes into
pg_proc.prorettype.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier-2
On Sat, Feb 10, 2018 at 01:46:40PM -0500, Tom Lane wrote:
> I pushed a fix for all that.

Shouldn't there be a test case as well?  The patch I sent upthread was
doing the whole set, except that I did not bother

> The failure in pg_get_functiondef() is still there.  While the immediate
> answer probably is to teach that function to emit correct CREATE PROCEDURE
> syntax, I continue to think that it's a bad idea to be putting zeroes into
> pg_proc.prorettype.

Yeah, or an error with a new function dedicated to procedures.  I also
finc confusing the use of prorettype to track this object type.

This brings the amount of objects stored in pg_proc to four.  Perhaps it
would be time to bring more clarity in pg_proc by introducing a prokind
column for functions, aggregates, window functions and procedures?  I
don't feel really hot for an extra boolean column like proisproc.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Michael Paquier-2
On Sun, Feb 11, 2018 at 08:17:55AM +0900, Michael Paquier wrote:
> On Sat, Feb 10, 2018 at 01:46:40PM -0500, Tom Lane wrote:
>> I pushed a fix for all that.
>
> Shouldn't there be a test case as well?  The patch I sent upthread was
> doing the whole set, except that I did not bother

... Rename EXPR_KIND_CALL to something else.

This was missing the last part of the sentence.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Tom Lane-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> writes:
> On Sat, Feb 10, 2018 at 01:46:40PM -0500, Tom Lane wrote:
>> I pushed a fix for all that.

> Shouldn't there be a test case as well?

There was one for the premature-free issue in d02d4a6d4.  I didn't really
see a need for an explicit test for the subselect issue.

> This brings the amount of objects stored in pg_proc to four.  Perhaps it
> would be time to bring more clarity in pg_proc by introducing a prokind
> column for functions, aggregates, window functions and procedures?

Yeah.  I was under the impression that Peter was looking into that ...
[ digs... ] see
https://www.postgresql.org/message-id/80ee1f5c-fa9d-7285-ed07-cff53d4f4858@...

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Peter Eisentraut-6
In reply to this post by Tom Lane-2
On 2/9/18 09:42, Tom Lane wrote:
> Meh.  It doesn't look significantly different to me than the restriction
> that you can't have sub-selects in CHECK expressions, index expressions,
> etc.  Obviously we need a clean failure like you get for those cases.
> But otherwise it's an OK restriction that stems from exactly the same
> cause: we do not want to invoke the full planner in this context (and
> even if we did, we don't want to use the full executor to execute the
> result).

A close analogy is that EXECUTE parameters also don't accept subqueries.
 It would perhaps be nice if that could be made to work, but as
discussed it would require a bunch more work.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Pavel Stehule


2018-02-12 18:17 GMT+01:00 Peter Eisentraut <[hidden email]>:
On 2/9/18 09:42, Tom Lane wrote:
> Meh.  It doesn't look significantly different to me than the restriction
> that you can't have sub-selects in CHECK expressions, index expressions,
> etc.  Obviously we need a clean failure like you get for those cases.
> But otherwise it's an OK restriction that stems from exactly the same
> cause: we do not want to invoke the full planner in this context (and
> even if we did, we don't want to use the full executor to execute the
> result).

A close analogy is that EXECUTE parameters also don't accept subqueries.
 It would perhaps be nice if that could be made to work, but as
discussed it would require a bunch more work.

I can live with it. Should be well documented and explained.

Regards

Pavel
 

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: CALL stmt, ERROR: unrecognized node type: 113 bug

Peter Eisentraut-6
In reply to this post by Pavel Stehule
On 2/1/18 11:33, Pavel Stehule wrote:
> postgres=# \sf test
> ERROR:  cache lookup failed for type 0

Here is a patch set that adds procedure support to \ef and \sf.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-Fix-typo.patch (21K) Download Attachment
0002-Add-tests-for-pg_get_functiondef.patch (4K) Download Attachment
0003-Add-procedure-support-to-pg_get_functiondef.patch (9K) Download Attachment
12
Previous Thread Next Thread