]> granicus.if.org Git - postgresql/commitdiff
Fix a bug introduced when set-returning SQL functions were made inline-able:
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 14 Dec 2009 02:16:04 +0000 (02:16 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 14 Dec 2009 02:16:04 +0000 (02:16 +0000)
we have to cope with the possibility that the declared result rowtype contains
dropped columns.  This fails in 8.4, as per bug #5240.

While at it, be more paranoid about inserting binary coercions when inlining.
The pre-8.4 code did not really need to worry about that because it could not
inline at all in any case where an added coercion could change the behavior
of the function's statement.  However, when inlining a SRF we allow sorting,
grouping, and set-ops such as UNION.  In these cases, modifying one of the
targetlist entries that the sort/group/setop depends on could conceivably
change the behavior of the function's statement --- so don't inline when
such a case applies.

src/backend/catalog/pg_proc.c
src/backend/executor/execQual.c
src/backend/executor/functions.c
src/backend/optimizer/util/clauses.c
src/include/executor/functions.h
src/test/regress/expected/rangefuncs.out
src/test/regress/sql/rangefuncs.sql

index d9c776773aaf2fc0e35a429fc6f53462ac8b4b50..397482d7f813602442cc9a3907dafd104d8a179e 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.164.2.1 2009/10/02 18:13:10 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.164.2.2 2009/12/14 02:16:03 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -744,7 +744,7 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
                                                                                                  proc->pronargs);
                        (void) check_sql_fn_retval(funcoid, proc->prorettype,
                                                                           querytree_list,
-                                                                          false, NULL);
+                                                                          NULL, NULL);
                }
                else
                        querytree_list = pg_parse_query(prosrc);
index b1bd9e4e7e157d6cbbfc00d1981e258ed72ac3f8..7cac1060afe2661a7c809fc5b7dc2a1637f86845 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.250 2009/06/11 17:25:38 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.250.2.1 2009/12/14 02:16:03 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -46,6 +46,7 @@
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/planner.h"
+#include "parser/parse_coerce.h"
 #include "pgstat.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
@@ -1344,7 +1345,7 @@ tupledesc_match(TupleDesc dst_tupdesc, TupleDesc src_tupdesc)
                Form_pg_attribute dattr = dst_tupdesc->attrs[i];
                Form_pg_attribute sattr = src_tupdesc->attrs[i];
 
-               if (dattr->atttypid == sattr->atttypid)
+               if (IsBinaryCoercible(sattr->atttypid, dattr->atttypid))
                        continue;                       /* no worries */
                if (!dattr->attisdropped)
                        ereport(ERROR,
index fe25798a21e2c8d94422211e6c69e1f8772678ef..2f3f4971db9b4132ce6c336826dfa96ea193641f 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/functions.c,v 1.135 2009/06/11 17:25:38 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/functions.c,v 1.135.2.1 2009/12/14 02:16:03 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -338,7 +338,7 @@ init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
        fcache->returnsTuple = check_sql_fn_retval(foid,
                                                                                           rettype,
                                                                                           queryTree_list,
-                                                                                          false,
+                                                                                          NULL,
                                                                                           &fcache->junkFilter);
 
        if (fcache->returnsTuple)
@@ -998,14 +998,27 @@ ShutdownSQLFunction(Datum arg)
  * function definition of a polymorphic function.)
  *
  * This function returns true if the sql function returns the entire tuple
- * result of its final statement, and false otherwise. Note that because we
- * allow "SELECT rowtype_expression", this may be false even when the declared
- * function return type is a rowtype.
+ * result of its final statement, or false if it returns just the first column
+ * result of that statement.  It throws an error if the final statement doesn't
+ * return the right type at all.
  *
- * If insertRelabels is true, then binary-compatible cases are dealt with
- * by actually inserting RelabelType nodes into the output targetlist;
- * obviously the caller must pass a parsetree that it's okay to modify in this
- * case.
+ * Note that because we allow "SELECT rowtype_expression", the result can be
+ * false even when the declared function return type is a rowtype.
+ *
+ * If modifyTargetList isn't NULL, the function will modify the final
+ * statement's targetlist in two cases:
+ * (1) if the tlist returns values that are binary-coercible to the expected
+ * type rather than being exactly the expected type.  RelabelType nodes will
+ * be inserted to make the result types match exactly.
+ * (2) if there are dropped columns in the declared result rowtype.  NULL
+ * output columns will be inserted in the tlist to match them.
+ * (Obviously the caller must pass a parsetree that is okay to modify when
+ * using this flag.)  Note that this flag does not affect whether the tlist is
+ * considered to be a legal match to the result type, only how we react to
+ * allowed not-exact-match cases.  *modifyTargetList will be set true iff
+ * we had to make any "dangerous" changes that could modify the semantics of
+ * the statement.  If it is set true, the caller should not use the modified
+ * statement, but for simplicity we apply the changes anyway.
  *
  * If junkFilter isn't NULL, then *junkFilter is set to a JunkFilter defined
  * to convert the function's tuple result to the correct output tuple type.
@@ -1014,10 +1027,11 @@ ShutdownSQLFunction(Datum arg)
  */
 bool
 check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
-                                       bool insertRelabels,
+                                       bool *modifyTargetList,
                                        JunkFilter **junkFilter)
 {
        Query      *parse;
+       List      **tlist_ptr;
        List       *tlist;
        int                     tlistlen;
        char            fn_typtype;
@@ -1026,6 +1040,8 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 
        AssertArg(!IsPolymorphicType(rettype));
 
+       if (modifyTargetList)
+               *modifyTargetList = false;      /* initialize for no change */
        if (junkFilter)
                *junkFilter = NULL;             /* initialize in case of VOID result */
 
@@ -1059,6 +1075,7 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                parse->utilityStmt == NULL &&
                parse->intoClause == NULL)
        {
+               tlist_ptr = &parse->targetList;
                tlist = parse->targetList;
        }
        else if (parse &&
@@ -1067,6 +1084,7 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                          parse->commandType == CMD_DELETE) &&
                         parse->returningList)
        {
+               tlist_ptr = &parse->returningList;
                tlist = parse->returningList;
        }
        else
@@ -1127,11 +1145,16 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                                        format_type_be(rettype)),
                                         errdetail("Actual return type is %s.",
                                                           format_type_be(restype))));
-               if (insertRelabels && restype != rettype)
+               if (modifyTargetList && restype != rettype)
+               {
                        tle->expr = (Expr *) makeRelabelType(tle->expr,
                                                                                                 rettype,
                                                                                                 -1,
                                                                                                 COERCE_DONTCARE);
+                       /* Relabel is dangerous if TLE is a sort/group or setop column */
+                       if (tle->ressortgroupref != 0 || parse->setOperations)
+                               *modifyTargetList = true;
+               }
 
                /* Set up junk filter if needed */
                if (junkFilter)
@@ -1144,6 +1167,8 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                int                     tupnatts;       /* physical number of columns in tuple */
                int                     tuplogcols; /* # of nondeleted columns in tuple */
                int                     colindex;       /* physical column index */
+               List       *newtlist;   /* new non-junk tlist entries */
+               List       *junkattrs;  /* new junk tlist entries */
 
                /*
                 * If the target list is of length 1, and the type of the varnode in
@@ -1160,11 +1185,16 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                        restype = exprType((Node *) tle->expr);
                        if (IsBinaryCoercible(restype, rettype))
                        {
-                               if (insertRelabels && restype != rettype)
+                               if (modifyTargetList && restype != rettype)
+                               {
                                        tle->expr = (Expr *) makeRelabelType(tle->expr,
                                                                                                                 rettype,
                                                                                                                 -1,
                                                                                                                 COERCE_DONTCARE);
+                                       /* Relabel is dangerous if sort/group or setop column */
+                                       if (tle->ressortgroupref != 0 || parse->setOperations)
+                                               *modifyTargetList = true;
+                               }
                                /* Set up junk filter if needed */
                                if (junkFilter)
                                        *junkFilter = ExecInitJunkFilter(tlist, false, NULL);
@@ -1188,11 +1218,14 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                /*
                 * Verify that the targetlist matches the return tuple type. We scan
                 * the non-deleted attributes to ensure that they match the datatypes
-                * of the non-resjunk columns.
+                * of the non-resjunk columns.  For deleted attributes, insert NULL
+                * result columns if the caller asked for that.
                 */
                tupnatts = tupdesc->natts;
                tuplogcols = 0;                 /* we'll count nondeleted cols as we go */
                colindex = 0;
+               newtlist = NIL;                 /* these are only used if modifyTargetList */
+               junkattrs = NIL;
 
                foreach(lc, tlist)
                {
@@ -1202,7 +1235,11 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                        Oid                     atttype;
 
                        if (tle->resjunk)
+                       {
+                               if (modifyTargetList)
+                                       junkattrs = lappend(junkattrs, tle);
                                continue;
+                       }
 
                        do
                        {
@@ -1214,6 +1251,26 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                                                                        format_type_be(rettype)),
                                        errdetail("Final statement returns too many columns.")));
                                attr = tupdesc->attrs[colindex - 1];
+                               if (attr->attisdropped && modifyTargetList)
+                               {
+                                       Expr   *null_expr;
+
+                                       /* The type of the null we insert isn't important */
+                                       null_expr = (Expr *) makeConst(INT4OID,
+                                                                                                  -1,
+                                                                                                  sizeof(int32),
+                                                                                                  (Datum) 0,
+                                                                                                  true,                /* isnull */
+                                                                                                  true /* byval */ );
+                                       newtlist = lappend(newtlist,
+                                                                          makeTargetEntry(null_expr,
+                                                                                                          colindex,
+                                                                                                          NULL,
+                                                                                                          false));
+                                       /* NULL insertion is dangerous in a setop */
+                                       if (parse->setOperations)
+                                               *modifyTargetList = true;
+                               }
                        } while (attr->attisdropped);
                        tuplogcols++;
 
@@ -1228,28 +1285,66 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                                                                   format_type_be(tletype),
                                                                   format_type_be(atttype),
                                                                   tuplogcols)));
-                       if (insertRelabels && tletype != atttype)
-                               tle->expr = (Expr *) makeRelabelType(tle->expr,
-                                                                                                        atttype,
-                                                                                                        -1,
-                                                                                                        COERCE_DONTCARE);
+                       if (modifyTargetList)
+                       {
+                               if (tletype != atttype)
+                               {
+                                       tle->expr = (Expr *) makeRelabelType(tle->expr,
+                                                                                                                atttype,
+                                                                                                                -1,
+                                                                                                                COERCE_DONTCARE);
+                                       /* Relabel is dangerous if sort/group or setop column */
+                                       if (tle->ressortgroupref != 0 || parse->setOperations)
+                                               *modifyTargetList = true;
+                               }
+                               tle->resno = colindex;
+                               newtlist = lappend(newtlist, tle);
+                       }
                }
 
-               for (;;)
+               /* remaining columns in tupdesc had better all be dropped */
+               for (colindex++; colindex <= tupnatts; colindex++)
                {
-                       colindex++;
-                       if (colindex > tupnatts)
-                               break;
                        if (!tupdesc->attrs[colindex - 1]->attisdropped)
-                               tuplogcols++;
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+                                                errmsg("return type mismatch in function declared to return %s",
+                                                               format_type_be(rettype)),
+                                                errdetail("Final statement returns too few columns.")));
+                       if (modifyTargetList)
+                       {
+                               Expr   *null_expr;
+
+                               /* The type of the null we insert isn't important */
+                               null_expr = (Expr *) makeConst(INT4OID,
+                                                                                          -1,
+                                                                                          sizeof(int32),
+                                                                                          (Datum) 0,
+                                                                                          true,                /* isnull */
+                                                                                          true /* byval */ );
+                               newtlist = lappend(newtlist,
+                                                                  makeTargetEntry(null_expr,
+                                                                                                  colindex,
+                                                                                                  NULL,
+                                                                                                  false));
+                               /* NULL insertion is dangerous in a setop */
+                               if (parse->setOperations)
+                                       *modifyTargetList = true;
+                       }
                }
 
-               if (tlistlen != tuplogcols)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-                        errmsg("return type mismatch in function declared to return %s",
-                                       format_type_be(rettype)),
-                                        errdetail("Final statement returns too few columns.")));
+               if (modifyTargetList)
+               {
+                       /* ensure resjunk columns are numbered correctly */
+                       foreach(lc, junkattrs)
+                       {
+                               TargetEntry *tle = (TargetEntry *) lfirst(lc);
+
+                               tle->resno = colindex++;
+                       }
+                       /* replace the tlist with the modified one */
+                       *tlist_ptr = list_concat(newtlist, junkattrs);
+               }
 
                /* Set up junk filter if needed */
                if (junkFilter)
index 75c5d0c94d095c0d34c1d7df697031f8010014e8..ddb1e22b681efbd89933e5623ff7049361d1c268 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.277 2009/06/11 14:48:59 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.277.2.1 2009/12/14 02:16:03 tgl Exp $
  *
  * HISTORY
  *       AUTHOR                        DATE                    MAJOR EVENT
@@ -3484,6 +3484,7 @@ inline_function(Oid funcid, Oid result_type, List *args,
        char       *src;
        Datum           tmp;
        bool            isNull;
+       bool            modifyTargetList;
        MemoryContext oldcxt;
        MemoryContext mycxt;
        ErrorContextCallback sqlerrcontext;
@@ -3603,7 +3604,7 @@ inline_function(Oid funcid, Oid result_type, List *args,
         * needed; that's probably not important, but let's be careful.
         */
        if (check_sql_fn_retval(funcid, result_type, list_make1(querytree),
-                                                       true, NULL))
+                                                       &modifyTargetList, NULL))
                goto fail;                              /* reject whole-tuple-result cases */
 
        /* Now we can grab the tlist expression */
@@ -3611,6 +3612,8 @@ inline_function(Oid funcid, Oid result_type, List *args,
 
        /* Assert that check_sql_fn_retval did the right thing */
        Assert(exprType(newexpr) == result_type);
+       /* It couldn't have made any dangerous tlist changes, either */
+       Assert(!modifyTargetList);
 
        /*
         * Additional validity checks on the expression.  It mustn't return a set,
@@ -3898,6 +3901,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
        char       *src;
        Datum           tmp;
        bool            isNull;
+       bool            modifyTargetList;
        MemoryContext oldcxt;
        MemoryContext mycxt;
        ErrorContextCallback sqlerrcontext;
@@ -4045,8 +4049,8 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
         * Make sure the function (still) returns what it's declared to.  This
         * will raise an error if wrong, but that's okay since the function would
         * fail at runtime anyway.      Note that check_sql_fn_retval will also insert
-        * RelabelType(s) if needed to make the tlist expression(s) match the
-        * declared type of the function.
+        * RelabelType(s) and/or NULL columns if needed to make the tlist
+        * expression(s) match the declared type of the function.
         *
         * If the function returns a composite type, don't inline unless the check
         * shows it's returning a whole tuple result; otherwise what it's
@@ -4054,11 +4058,19 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
         */
        if (!check_sql_fn_retval(fexpr->funcid, fexpr->funcresulttype,
                                                         querytree_list,
-                                                        true, NULL) &&
+                                                        &modifyTargetList, NULL) &&
                (get_typtype(fexpr->funcresulttype) == TYPTYPE_COMPOSITE ||
                 fexpr->funcresulttype == RECORDOID))
                goto fail;                              /* reject not-whole-tuple-result cases */
 
+       /*
+        * If we had to modify the tlist to make it match, and the statement is
+        * one in which changing the tlist contents could change semantics, we
+        * have to punt and not inline.
+        */
+       if (modifyTargetList)
+               goto fail;
+
        /*
         * If it returns RECORD, we have to check against the column type list
         * provided in the RTE; check_sql_fn_retval can't do that.  (If no match,
index 10435f8617e792bf0ee07c25de34d613d5bcd687..15aea1b39774fe8328bf73397917784170959226 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/functions.h,v 1.33 2009/01/01 17:23:59 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/executor/functions.h,v 1.33.2.1 2009/12/14 02:16:03 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -22,7 +22,7 @@ extern Datum fmgr_sql(PG_FUNCTION_ARGS);
 
 extern bool check_sql_fn_retval(Oid func_id, Oid rettype,
                                        List *queryTreeList,
-                                       bool insertRelabels,
+                                       bool *modifyTargetList,
                                        JunkFilter **junkFilter);
 
 extern DestReceiver *CreateSQLFunctionDestReceiver(void);
index 486dd3f3fe06f835cf9e7d414645937fc2dc3c6a..e8bdc42b9b660fafd3c034db3b1267cd9001cbf3 100644 (file)
@@ -830,3 +830,64 @@ ERROR:  a column definition list is required for functions returning "record"
 LINE 1: select * from testfoo();
                       ^
 drop function testfoo();
+--
+-- Check some cases involving dropped columns in a rowtype result
+--
+create temp table users (userid text, email text, todrop bool, enabled bool);
+insert into users values ('id','email',true,true);
+insert into users values ('id2','email2',true,true);
+alter table users drop column todrop;
+create or replace function get_first_user() returns users as
+$$ SELECT * FROM users ORDER BY userid LIMIT 1; $$
+language sql stable;
+SELECT get_first_user();
+ get_first_user 
+----------------
+ (id,email,t)
+(1 row)
+
+SELECT * FROM get_first_user();
+ userid | email | enabled 
+--------+-------+---------
+ id     | email | t
+(1 row)
+
+create or replace function get_users() returns setof users as
+$$ SELECT * FROM users ORDER BY userid; $$
+language sql stable;
+SELECT get_users();
+   get_users    
+----------------
+ (id,email,t)
+ (id2,email2,t)
+(2 rows)
+
+SELECT * FROM get_users();
+ userid | email  | enabled 
+--------+--------+---------
+ id     | email  | t
+ id2    | email2 | t
+(2 rows)
+
+drop function get_first_user();
+drop function get_users();
+drop table users;
+-- this won't get inlined because of type coercion, but it shouldn't fail
+create or replace function foobar() returns setof text as
+$$ select 'foo'::varchar union all select 'bar'::varchar ; $$
+language sql stable;
+select foobar();
+ foobar 
+--------
+ foo
+ bar
+(2 rows)
+
+select * from foobar();
+ foobar 
+--------
+ foo
+ bar
+(2 rows)
+
+drop function foobar();
index 3727a36aaff501e3f4e3474fa19f7496015c349e..d780c6cd3ebed32e7a07bf081df0a8becb89fa14 100644 (file)
@@ -385,3 +385,41 @@ select * from testfoo() as t(f1 int8,f2 int8);
 select * from testfoo(); -- fail
 
 drop function testfoo();
+
+--
+-- Check some cases involving dropped columns in a rowtype result
+--
+
+create temp table users (userid text, email text, todrop bool, enabled bool);
+insert into users values ('id','email',true,true);
+insert into users values ('id2','email2',true,true);
+alter table users drop column todrop;
+
+create or replace function get_first_user() returns users as
+$$ SELECT * FROM users ORDER BY userid LIMIT 1; $$
+language sql stable;
+
+SELECT get_first_user();
+SELECT * FROM get_first_user();
+
+create or replace function get_users() returns setof users as
+$$ SELECT * FROM users ORDER BY userid; $$
+language sql stable;
+
+SELECT get_users();
+SELECT * FROM get_users();
+
+drop function get_first_user();
+drop function get_users();
+drop table users;
+
+-- this won't get inlined because of type coercion, but it shouldn't fail
+
+create or replace function foobar() returns setof text as
+$$ select 'foo'::varchar union all select 'bar'::varchar ; $$
+language sql stable;
+
+select foobar();
+select * from foobar();
+
+drop function foobar();