8000 Fix list-munging bug that broke SQL function result coercions. · postgrespro/postgres_cluster@25378db · GitHub
[go: up one dir, main page]
Skip to content

Commit 25378db

Browse files
tglsfdctglsfdccommitted
Fix list-munging bug that broke SQL function result coercions.
Since commit 913bbd8, check_sql_fn_retval() can either insert type coercion steps in-line in the Query that produces the SQL function's results, or generate a new top-level Query to perform the coercions, if modifying the Query's output in-place wouldn't be safe. However, it appears that the latter case has never actually worked, because the code tried to inject the new Query back into the query list it was passed ... which is not the list that will be used for later processing when we execute the SQL function "normally" (without inlining it). So we ended up with no coercion happening at run-time, leading to wrong results or crashes depending on the datatypes involved. While the regression tests look like they cover this area well enough, through a huge bit of bad luck all the test cases that exercise the separate-Query path were checking either inline-able cases (which accidentally didn't have the bug) or cases that are no-ops at runtime (e.g., varchar to text), so that the failure to perform the coercion wasn't obvious. The fact that the cases that don't work weren't allowed at all before v13 probably contributed to not noticing the problem sooner, too. To fix, get rid of the separate "flat" list of Query nodes and instead pass the real two-level list that is going to be used later. I chose to make the same change in check_sql_fn_statements(), although that has no actual bug, just so that we don't need that data structure at all. This is an API change, as evidenced by the adjustments needed to callers outside functions.c. That's a bit scary to be doing in a released branch, but so far as I can tell from a quick search, there are no outside callers of these functions (and they are sufficiently specific to our semantics for SQL-language functions that it's not apparent why any extension would need to call them). In any case, v13 already changed the API of check_sql_fn_retval() compared to prior branches. Per report from pinker. Back-patch to v13 where this code came in. Discussion: https://postgr.es/m/1603050466566-0.post@n3.nabble.com
1 parent 33acc6b commit 25378db

File tree

6 files changed

+132
-60
lines changed

6 files changed

+132
-60
lines changed

‎src/backend/catalog/pg_proc.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -918,8 +918,8 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
918918
(ParserSetupHook) sql_fn_parser_setup,
919919
pinfo,
920920
NULL);
921-
querytree_list = list_concat(querytree_list,
922-
querytree_sublist);
921+
querytree_list = lappend(querytree_list,
922+
querytree_sublist);
923923
}
924924

925925
check_sql_fn_statements(querytree_list);

‎src/backend/executor/functions.c

Lines changed: 69 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -608,7 +608,6 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
608608
SQLFunctionCachePtr fcache;
609609
List *raw_parsetree_list;
610610
List *queryTree_list;
611-
List *flat_query_list;
612611
List *resulttlist;
613612
ListCell *lc;
614613
Datum tmp;
@@ -688,13 +687,7 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
688687

689688
/*
690689
* Parse and rewrite the queries in the function text. Use sublists to
691-
* keep track of the original query boundaries. But we also build a
692-
* "flat" list of the rewritten queries to pass to check_sql_fn_retval.
693-
* This is because the last canSetTag query determines the result type
694-
* independently of query boundaries --- and it might not be in the last
695-
* sublist, for example if the last query rewrites to DO INSTEAD NOTHING.
696-
* (It might not be unreasonable to throw an error in such a case, but
697-
* this is the historical behavior and it doesn't seem worth changing.)
690+
* keep track of the original query boundaries.
698691
*
699692
* Note: since parsing and planning is done in fcontext, we will generate
700693
* a lot of cruft that lives as long as the fcache does. This is annoying
@@ -704,7 +697,6 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
704697
raw_parsetree_list = pg_parse_query(fcache->src);
705698

706699
queryTree_list = NIL;
707-
flat_query_list = NIL;
708700
foreach(lc, raw_parsetree_list)
709701
{
710702
RawStmt *parsetree = lfirst_node(RawStmt, lc);
@@ -716,10 +708,12 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
716708
fcache->pinfo,
717709
NULL);
718710
queryTree_list = lappend(queryTree_list, queryTree_sublist);
719-
flat_query_list = list_concat(flat_query_list, queryTree_sublist);
720711
}
721712

722-
check_sql_fn_statements(flat_query_list);
713+
/*
714+
* Check that there are no statements we don't want to allow.
715+
*/
716+
check_sql_fn_statements(queryTree_list);
723717

724718
/*
725719
* Check that the function returns the type it claims to. Although in
@@ -739,7 +733,7 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
739733
* the rowtype column into multiple columns, since we have no way to
740734
* notify the caller that it should do that.)
741735
*/
742-
fcache->returnsTuple = check_sql_fn_retval(flat_query_list,
736+
fcache->returnsTuple = check_sql_fn_retval(queryTree_list,
743737
rettype,
744738
rettupdesc,
745739
false,
@@ -1509,51 +1503,63 @@ ShutdownSQLFunction(Datum arg)
15091503
* is not acceptable.
15101504
*/
15111505
void
1512-
check_sql_fn_statements(List *queryTreeList)
1506+
check_sql_fn_statements(List *queryTreeLists)
15131507
{
15141508
ListCell *lc;
15151509

1516-
foreach(lc, queryTreeList)
1510+
/* We are given a list of sublists of Queries */
1511+
foreach(lc, queryTreeLists)
15171512
{
1518-
Query *query = lfirst_node(Query, lc);
1513+
List *sublist = lfirst_node(List, lc);
1514+
ListCell *lc2;
15191515

1520-
/*
1521-
* Disallow procedures with output arguments. The current
1522-
* implementation would just throw the output values away, unless the
1523-
* statement is the last one. Per SQL standard, we should assign the
1524-
* output values by name. By disallowing this here, we preserve an
1525-
* opportunity for future improvement.
1526-
*/
1527-
if (query->commandType == CMD_UTILITY &&
1528-
IsA(query->utilityStmt, CallStmt))
1516+
foreach(lc2, sublist)
15291517
{
1530-
CallStmt *stmt = castNode(CallStmt, query->utilityStmt);
1531-
HeapTuple tuple;
1532-
int numargs;
1533-
Oid *argtypes;
1534-
char **argnames;
1535-
char *argmodes;
1536-
int i;
1537-
1538-
tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(stmt->funcexpr->funcid));
1539-
if (!HeapTupleIsValid(tuple))
1540-
elog(ERROR, "cache lookup failed for function %u", stmt->funcexpr->funcid);
1541-
numargs = get_func_arg_info(tuple, &argtypes, &argnames, &argmodes);
1542-
ReleaseSysCache(tuple);
1543-
1544-
for (i = 0; i < numargs; i++)
1518+
Query *query = lfirst_node(Query, lc2);
1519+
1520+
/*
1521+
* Disallow procedures with output arguments. The current
1522+
* implementation would just throw the output values away, unless
1523+
* the statement is the last one. Per SQL standard, we should
1524+
* assign the output values by name. By disallowing this here, we
1525+
* preserve an opportunity for future improvement.
1526+
*/
1527+
if (query->commandType == CMD_UTILITY &&
1528+
IsA(query->utilityStmt, CallStmt))
15451529
{
1546-
if (argmodes && (argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_OUT))
1547-
ereport(ERROR,
1548-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
1549-
errmsg("calling procedures with output arguments is not supported in SQL functions")));
1530+
CallStmt *stmt = castNode(CallStmt, query->utilityStmt);
1531+
HeapTuple tuple;
1532+
int numargs;
1533+
Oid *argtypes;
1534+
char **argnames;
1535+
char *argmodes;
1536+
int i;
1537+
1538+
tuple = SearchSysCache1(PROCOID,
1539+
ObjectIdGetDatum(stmt->funcexpr->funcid));
1540+
if (!HeapTupleIsValid(tuple))
1541+
elog(ERROR, "cache lookup failed for function %u",
1542+
stmt->funcexpr->funcid);
1543+
numargs = get_func_arg_info(tuple,
1544+
&argtypes, &argnames, &argmodes);
1545+
ReleaseSysCache(tuple);
1546+
1547+
for (i = 0; i < numargs; i++)
1548+
{
1549+
if (argmodes && (argmodes[i] == PROARGMODE_INOUT ||
1550+
argmodes[i] == PROARGMODE_OUT))
1551+
ereport(ERROR,
1552+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
1553+
errmsg("calling procedures with output arguments is not supported in SQL functions")));
1554+
}
15501555
}
15511556
}
15521557
}
15531558
}
15541559

15551560
/*
1556-
* check_sql_fn_retval() -- check return value of a list of sql parse trees.
1561+
* check_sql_fn_retval()
1562+
* Check return value of a list of lists of sql parse trees.
15571563
*
15581564
* The return value of a sql function is the value returned by the last
15591565
* canSetTag query in the function. We do some ad-hoc type checking and
@@ -1591,7 +1597,7 @@ check_sql_fn_statements(List *queryTreeList)
15911597
* function is defined to return VOID then *resultTargetList is set to NIL.
15921598
*/
15931599
bool
1594-
check_sql_fn_retval(List *queryTreeList,
1600+
check_sql_fn_retval(List *queryTreeLists,
15951601
Oid rettype, TupleDesc rettupdesc,
15961602
bool insertDroppedCols,
15971603
List **resultTargetList)
@@ -1618,20 +1624,30 @@ check_sql_fn_retval(List *queryTreeList,
16181624
return false;
16191625

16201626
/*
1621-
* Find the last canSetTag query in the list. This isn't necessarily the
1622-
* last parsetree, because rule rewriting can insert queries after what
1623-
* the user wrote.
1627+
* Find the last canSetTag query in the function body (which is presented
1628+
* to us as a list of sublists of Query nodes). This isn't necessarily
1629+
* the last parsetree, because rule rewriting can insert queries after
1630+
* what the user wrote. Note that it might not even be in the last
1631+
* sublist, for example if the last query rewrites to DO INSTEAD NOTHING.
1632+
* (It might not be unreasonable to throw an error in such a case, but
1633+
* this is the historical behavior and it doesn't seem worth changing.)
16241634
*/
16251635
parse = NULL;
16261636
parse_cell = NULL;
1627-
foreach(lc, queryTreeList)
1637+
foreach(lc, queryTreeLists)
16281638
{
1629-
Query *q = lfirst_node(Query, lc);
1639+
List *sublist = lfirst_node(List, lc);
1640+
ListCell *lc2;
16301641

1631-
if (q->canSetTag)
1642+
foreach(lc2, sublist)
16321643
{
1633-
parse = q;
1634-
parse_cell = lc;
1644+
Query *q = lfirst_node(Query, lc2);
1645+
1646+
if (q->canSetTag)
1647+
{
1648+
parse = q;
1649+
parse_cell = lc2;
1650+
}
16351651
}
16361652
}
16371653

‎src/backend/optimizer/util/clauses.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4522,7 +4522,8 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
45224522
* needed; that's probably not important, but let's be careful.
45234523
*/
45244524
querytree_list = list_make1(querytree);
4525-
if (check_sql_fn_retval(querytree_list, result_type, rettupdesc,
4525+
if (check_sql_fn_retval(list_make1(querytree_list),
4526+
result_type, rettupdesc,
45264527
false, NULL))
45274528
goto fail; /* reject whole-tuple-result cases */
45284529

@@ -5040,7 +5041,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
50405041
* shows it's returning a whole tuple result; otherwise what it's
50415042
* returning is a single composite column which is not what we need.
50425043
*/
5043-
if (!check_sql_fn_retval(querytree_list,
5044+
if (!check_sql_fn_retval(list_make1(querytree_list),
50445045
fexpr->funcresulttype, rettupdesc,
50455046
true, NULL) &&
50465047
(functypclass == TYPEFUNC_COMPOSITE ||
@@ -5052,7 +5053,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
50525053
* check_sql_fn_retval might've inserted a projection step, but that's
50535054
* fine; just make sure we use the upper Query.
50545055
*/
5055-
querytree = linitial(querytree_list);
5056+
querytree = linitial_node(Query, querytree_list);
50565057

50575058
/*
50585059
* Looks good --- substitute parameters into the query.

‎src/include/executor/functions.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ extern SQLFunctionParseInfoPtr prepare_sql_fn_parse_info(HeapTuple procedureTupl
2929
extern void sql_fn_parser_setup(struct ParseState *pstate,
3030
SQLFunctionParseInfoPtr pinfo);
3131

32-
extern void check_sql_fn_statements(List *queryTreeList);
32+
extern void check_sql_fn_statements(List *queryTreeLists);
3333

34-
extern bool check_sql_fn_retval(List *queryTreeList,
34+
extern bool check_sql_fn_retval(List *queryTreeLists,
3535
Oid rettype, TupleDesc rettupdesc,
3636
bool insertDroppedCols,
3737
List **resultTargetList);

‎src/test/regress/expected/rangefuncs.out

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2109,6 +2109,50 @@ select * from testrngfunc();
21092109
7.136178 | 7.14
21102110
(1 row)
21112111

2112+
create or replace function testrngfunc() returns setof rngfunc_type as $$
2113+
select 1, 2 union select 3, 4 order by 1;
2114+
$$ language sql immutable;
2115+
explain (verbose, costs off)
2116+
select testrngfunc();
2117+
QUERY PLAN
2118+
-------------------------
2119+
ProjectSet
2120+
Output: testrngfunc()
2121+
-> Result
2122+
(3 rows)
2123+
2124+
select testrngfunc();
2125+
testrngfunc
2126+
-----------------
2127+
(1.000000,2.00)
2128+
(3.000000,4.00)
2129+
(2 rows)
2130+
2131+
explain (verbose, costs off)
2132+
select * from testrngfunc();
2133+
QUERY PLAN
2134+
----------------------------------------------------------
2135+
Subquery Scan on "*SELECT*"
2136+
Output: "*SELECT*"."?column?", "*SELECT*"."?column?_1"
2137+
-> Unique
2138+
Output: (1), (2)
2139+
-> Sort
2140+
Output: (1), (2)
2141+
Sort Key: (1), (2)
2142+
-> Append
2143+
-> Result
2144+
Output: 1, 2
2145+
-> Result
2146+
Output: 3, 4
2147+
(12 rows)
2148+
2149+
select * from testrngfunc();
2150+
f1 | f2
2151+
----------+------
2152+
1.000000 | 2.00
2153+
3.000000 | 4.00
2154+
(2 rows)
2155+
21122156
drop type rngfunc_type cascade;
21132157
NOTICE: drop cascades to function testrngfunc()
21142158
--

‎src/test/regress/sql/rangefuncs.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -629,6 +629,17 @@ explain (verbose, costs off)
629629
select * from testrngfunc();
630630
select * from testrngfunc();
631631

632+
create or replace function testrngfunc() returns setof rngfunc_type as $$
633+
select 1, 2 union select 3, 4 order by 1;
634+
$$ language sql immutable;
635+
636+
explain (verbose, costs off)
637+
select testrngfunc();
638+
select testrngfunc();
639+
explain (verbose, costs off)
640+
select * from testrngfunc();
641+
select * from testrngfunc();
642+
632643
drop type rngfunc_type cascade;
633644

634645
--

0 commit comments

Comments
 (0)
0