Re: SRF patch (was Re: [HACKERS] troubleshooting pointers)

Tom Lane wrote:
>>I don't understand why this should be rejected, but it does fail for me
>>also, due to a NULL slot pointer. At what point should it be rejected?
>
>
> In the parser.  Ideally, fooid should not even be *visible* while we are
> parsing the arguments to the sibling FROM node.  Compare the handling of
> variable resolution in JOIN/ON clauses --- the namespace gets
> manipulated so that those clauses can't see vars from sibling FROM nodes.
>

Attached patch takes care of this case. It also passes my previous test
cases (see below). Applies cleanly to CVS tip and passes all regression
tests. Please apply if there are no objections.

I'm still working on the second test case from Tom (the NULL slot
pointer inducing subselect).

Joe

------< tests >-------
test=# \i /opt/src/srf-test.sql
DROP TABLE foo;
DROP
CREATE TABLE foo(fooid int, f2 int);
CREATE
INSERT INTO foo VALUES(1, 11);
INSERT 126218 1
INSERT INTO foo VALUES(2, 22);
INSERT 126219 1
INSERT INTO foo VALUES(1, 111);
INSERT 126220 1
DROP FUNCTION foot(int);
DROP
CREATE FUNCTION foot(int) returns setof foo as 'SELECT * FROM foo WHERE
fooid = $1;' LANGUAGE SQL;
CREATE

-- should fail with ERROR message
select * from foo, foot(fooid) z where foo.f2 = z.f2;
psql:/opt/src/srf-test.sql:10: ERROR:  Function relation in FROM clause
may not refer to other relation, "foo"

DROP TABLE foo;
DROP
CREATE TABLE foo (fooid int, foosubid int, fooname text, primary
key(fooid,foosubid));
psql:/opt/src/srf-test.sql:13: NOTICE:  CREATE TABLE / PRIMARY KEY will
create implicit index 'foo_pkey' for table 'foo'
CREATE
INSERT INTO foo VALUES(1,1,'Joe');
INSERT 126228 1
INSERT INTO foo VALUES(1,2,'Ed');
INSERT 126229 1
INSERT INTO foo VALUES(2,1,'Mary');
INSERT 126230 1

-- sql, proretset = f, prorettype = b
DROP FUNCTION getfoo(int);
DROP
CREATE FUNCTION getfoo(int) RETURNS int AS 'SELECT $1;' LANGUAGE SQL;
CREATE
SELECT * FROM getfoo(1) AS t1;
  getfoo
--------
       1
(1 row)

DROP VIEW vw_getfoo;
DROP
CREATE VIEW vw_getfoo AS SELECT * FROM getfoo(1);
CREATE
SELECT * FROM vw_getfoo;
  getfoo
--------
       1
(1 row)


-- sql, proretset = t, prorettype = b
DROP FUNCTION getfoo(int);
DROP
CREATE FUNCTION getfoo(int) RETURNS setof int AS 'SELECT fooid FROM foo
WHERE fooid = $1;' LANGUAGE SQL;
CREATE
SELECT * FROM getfoo(1) AS t1;
  getfoo
--------
       1
       1
(2 rows)

DROP VIEW vw_getfoo;
DROP
CREATE VIEW vw_getfoo AS SELECT * FROM getfoo(1);
CREATE
SELECT * FROM vw_getfoo;
  getfoo
--------
       1
       1
(2 rows)


-- sql, proretset = t, prorettype = b
DROP FUNCTION getfoo(int);
DROP
CREATE FUNCTION getfoo(int) RETURNS setof text AS 'SELECT fooname FROM
foo WHERE fooid = $1;' LANGUAGE SQL;
CREATE
SELECT * FROM getfoo(1) AS t1;
  getfoo
--------
  Joe
  Ed
(2 rows)

DROP VIEW vw_getfoo;
DROP
CREATE VIEW vw_getfoo AS SELECT * FROM getfoo(1);
CREATE
SELECT * FROM vw_getfoo;
  getfoo
--------
  Joe
  Ed
(2 rows)


-- sql, proretset = f, prorettype = c
DROP FUNCTION getfoo(int);
DROP
CREATE FUNCTION getfoo(int) RETURNS foo AS 'SELECT * FROM foo WHERE
fooid = $1;' LANGUAGE SQL;
CREATE
SELECT * FROM getfoo(1) AS t1;
  fooid | foosubid | fooname
-------+----------+---------
      1 |        1 | Joe
(1 row)

DROP VIEW vw_getfoo;
DROP
CREATE VIEW vw_getfoo AS SELECT * FROM getfoo(1);
CREATE
SELECT * FROM vw_getfoo;
  fooid | foosubid | fooname
-------+----------+---------
      1 |        1 | Joe
(1 row)


-- sql, proretset = t, prorettype = c
DROP FUNCTION getfoo(int);
DROP
CREATE FUNCTION getfoo(int) RETURNS setof foo AS 'SELECT * FROM foo
WHERE fooid = $1;' LANGUAGE SQL;
CREATE
SELECT * FROM getfoo(1) AS t1;
  fooid | foosubid | fooname
-------+----------+---------
      1 |        1 | Joe
      1 |        2 | Ed
(2 rows)

DROP VIEW vw_getfoo;
DROP
CREATE VIEW vw_getfoo AS SELECT * FROM getfoo(1);
CREATE
SELECT * FROM vw_getfoo;
  fooid | foosubid | fooname
-------+----------+---------
      1 |        1 | Joe
      1 |        2 | Ed
(2 rows)


-- C, proretset = f, prorettype = b
SELECT * FROM dblink_replace('123456789987654321', '99', 'HelloWorld');
        dblink_replace
----------------------------
  12345678HelloWorld87654321
(1 row)

DROP VIEW vw_dblink_replace;
DROP
CREATE VIEW vw_dblink_replace AS SELECT * FROM
dblink_replace('123456789987654321', '99', 'HelloWorld');
CREATE
SELECT * FROM vw_dblink_replace;
        dblink_replace
----------------------------
  12345678HelloWorld87654321
(1 row)


-- C, proretset = t, prorettype = b
SELECT dblink_get_pkey FROM dblink_get_pkey('foo');
  dblink_get_pkey
-----------------
  fooid
  foosubid
(2 rows)

DROP VIEW vw_dblink_get_pkey;
DROP
CREATE VIEW vw_dblink_get_pkey AS SELECT dblink_get_pkey FROM
dblink_get_pkey('foo');
CREATE
SELECT * FROM vw_dblink_get_pkey;
  dblink_get_pkey
-----------------
  fooid
  foosubid
(2 rows)

Index: src/backend/parser/parse_clause.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/backend/parser/parse_clause.c,v
retrieving revision 1.92
diff -c -r1.92 parse_clause.c
*** src/backend/parser/parse_clause.c    12 May 2002 23:43:03 -0000    1.92
--- src/backend/parser/parse_clause.c    15 May 2002 22:55:37 -0000
***************
*** 61,67 ****
  static List *addTargetToSortList(TargetEntry *tle, List *sortlist,
                      List *targetlist, List *opname);
  static bool exprIsInSortList(Node *expr, List *sortList, List *targetList);
!

  /*
   * transformFromClause -
--- 61,70 ----
  static List *addTargetToSortList(TargetEntry *tle, List *sortlist,
                      List *targetlist, List *opname);
  static bool exprIsInSortList(Node *expr, List *sortList, List *targetList);
! static void checkParameterVisibility(ParseState *pstate,
!                                 RangeTblRef *rtr,
!                                 RangeFunction *rangefunc,
!                                 List *containedRels);

  /*
   * transformFromClause -
***************
*** 545,550 ****
--- 548,559 ----

          rtr = transformRangeFunction(pstate, (RangeFunction *) n);
          *containedRels = makeListi1(rtr->rtindex);
+
+         /*
+          * Make sure we've only been given valid parameters, i.e.
+          * we should not see vars from sibling FROM nodes.
+          */
+         checkParameterVisibility(pstate, rtr, (RangeFunction *) n, *containedRels);
          return (Node *) rtr;
      }
      else if (IsA(n, JoinExpr))
***************
*** 1377,1380 ****
--- 1386,1445 ----
              return true;
      }
      return false;
+ }
+
+ /* checkParameterVisibility()
+  *      Make sure we don't try to access vars from
+  *      sibling FROM nodes as RangeFunction parameters.
+  */
+ static void
+ checkParameterVisibility(ParseState *pstate, RangeTblRef *rtr, RangeFunction *rangefunc, List *containedRels)
+ {
+     FuncCall       *funccallnode = (FuncCall *) rangefunc->funccallnode;
+     List           *args = funccallnode->args;
+     List           *save_namespace;
+     List           *clause_varnos,
+                    *l;
+
+     /*
+      * This is a tad tricky, for two reasons.  First, the namespace that
+      * the join expression should see is just the any outer references
+      * from upper pstate levels. So, temporarily set this pstate's namespace
+      * accordingly.  (We need not check for refname conflicts, because
+      * transformFromClauseItem() already did.) NOTE: this code is OK only
+      * because a RangeFunction can't legally alter the namespace by causing
+      * implicit relation refs to be added.
+      */
+     save_namespace = pstate->p_namespace;
+     pstate->p_namespace = makeList1(rtr);
+
+     /* transform the list of arguments */
+     foreach(args, funccallnode->args)
+     {
+         lfirst(args) = transformExpr(pstate, (Node *) lfirst(args));
+
+         /*
+          * Second, we need to check that the ON condition doesn't refer to any
+          * rels outside the input subtrees of the JOIN.  It could do that
+          * despite our hack on the namespace if it uses fully-qualified names.
+          * So, grovel through the transformed clause and make sure there are
+          * no bogus references.  (Outer references are OK, and are ignored
+          * here.)
+          */
+
+         clause_varnos = pull_varnos(lfirst(args));
+         foreach(l, clause_varnos)
+         {
+             int            varno = lfirsti(l);
+
+             if (!intMember(varno, containedRels))
+             {
+                 elog(ERROR, "Function relation in FROM clause may not refer to other relation, \"%s\"",
+                      rt_fetch(varno, pstate->p_rtable)->eref->aliasname);
+             }
+         }
+         freeList(clause_varnos);
+
+     }
+     pstate->p_namespace = save_namespace;
  }