udf: select * in a udf combined with edited table can cause crashes

[jordanlewis]

It's possible to use SELECT * within a UDF, which seems to cause issues after editing the table that's being SELECT *'d from. In other contexts we just ban SELECT *.

[email protected]:26257/defaultdb> create table t (a int);
CREATE TABLE
[email protected]:26257/defaultdb> create function f() returns int language sql as 'select * from t';
CREATE FUNCTION
[email protected]:26257/defaultdb> alter table t add column b int; 
ALTER TABLE
[email protected]:26257/defaultdb> select f();
   f
--------
  NULL
(1 row)
[email protected]:26257/defaultdb> insert into t values(1);
INSERT 0 1
[email protected]:26257/defaultdb> select f();
*
* ERROR: a SQL panic has occurred while executing the following statement:
* SELECT f()
*
*
* ERROR: a panic has occurred!
* runtime error: index out of range [1] with length 1
* (1) attached stack trace
*   -- stack trace:
*   | github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1
*   |   github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:806
*   | [...repeated from below...]
* Wraps: (2) while executing: SELECT f()
* Wraps: (3) attached stack trace
*   -- stack trace:
*   | github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1
*   |   github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:806
*   | runtime.gopanic
*   |   GOROOT/src/runtime/panic.go:838
*   | github.com/cockroachdb/cockroach/pkg/sql/colexecerror.CatchVectorizedRuntimeError.func1
*   |   github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:58
*   | runtime.gopanic
*   |   GOROOT/src/runtime/panic.go:838
*   | runtime.goPanicIndex
*   |   GOROOT/src/runtime/panic.go:89
*   | github.com/cockroachdb/cockroach/pkg/sql.(*rowContainerHelper).AddRow
*   |   github.com/cockroachdb/cockroach/pkg/sql/buffer_util.go:90
*   | github.com/cockroachdb/cockroach/pkg/sql.(*RowResultWriter).AddRow
*   |   github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:908
*   | github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLReceiver).Push
*   |   github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1241
*   | github.com/cockroachdb/cockroach/pkg/sql/execinfra.Run
*   |   github.com/cockroachdb/cockroach/pkg/sql/execinfra/base.go:189
*   | github.com/cockroachdb/cockroach/pkg/sql/execinfra.(*ProcessorBaseNoHelper).Run
*   |   github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:722

For the record, the equivalent sequence in Postgres does this:

jordan=> select f();
ERROR:  return type mismatch in function declared to return integer
DETAIL:  Final statement must return exactly one column.
CONTEXT:  SQL function "f" during startup

Jira issue: CRDB-18552

[chengxiong-ruan]

This is a good catch and interesting...In postgres if you use their early binding syntax, it still works:

postgres=# create table tt(a int);
CREATE TABLE

postgres=# create function ff() returns int language sql
postgres-# begin atomic
postgres-# select * from tt;
postgres-# end;
CREATE FUNCTION

postgres=# alter table tt add column b int;
ALTER TABLE

postgres=# insert into tt values (2,1);
INSERT 0 1

postgres=# select ff();
 ff
----
  2
(1 row)

I'm confused what should we do here for the long term. I think in the short term we should prevent the function from being broken. Either disallow the table from being mutated, or we should revalidate the function body when doing the type checking to throw out a better error.

[chengxiong-ruan]

This is also a good corner case to consider when we're going to design function body rewritting.

[ajwerner]

postgres resolves the * at definition time when early binding -- or, like we do for views, disallow them for this release and come back and attack that in 23.1 for both features.

SELECT n.nspname AS schema
      ,proname AS fname
      ,proargnames AS args
      ,t.typname AS return_type
      ,d.description
      ,pg_get_functiondef(p.oid) as definition
--      ,CASE WHEN NOT p.proisagg THEN pg_get_functiondef(p.oid)
--            ELSE 'pg_get_functiondef() can''t be used with aggregate functions'
--       END as definition
  FROM pg_proc p
  JOIN pg_type t
    ON p.prorettype = t.oid
  LEFT OUTER
  JOIN pg_description d
    ON p.oid = d.objoid
  LEFT OUTER
  JOIN pg_namespace n
    ON n.oid = p.pronamespace
 WHERE NOT p.prokind = 'a' and p.proname = 'f';
 schema | fname | args | return_type | description |                   definition                   
--------+-------+------+-------------+-------------+------------------------------------------------
 public | f     | {i}  | int4        |             | CREATE OR REPLACE FUNCTION public.f(i integer)+
        |       |      |             |             |  RETURNS integer                              +
        |       |      |             |             |  LANGUAGE sql                                 +
        |       |      |             |             |  IMMUTABLE                                    +
        |       |      |             |             | BEGIN ATOMIC                                  +
        |       |      |             |             |  SELECT t.i                                   +
        |       |      |             |             |     FROM sc.t                                 +
        |       |      |             |             |    WHERE (f.i > t.i);                         +
        |       |      |             |             | END                                           +
        |       |      |             |             | 

[chengxiong-ruan]

Interestingly we don't support * expression in views. I think @mgartner enabled it recently for UDF. But yeah, I agree that we can make it simple in 22.2.

[mgartner]

I'm planning on trying to fix this for 22.2.

[mgartner]

Labelling as a release blocker. A fix to prevent node crashes in this case should be trivial.

[ajwerner]

I think we can disable support for the * for this release and it would be okay. Postgres in views and in the eagerly resolved form of UDFs turns the * into column names at creation time. I think we should do that.