Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitc8ab970

Browse files
committed
Fix list-munging bug that broke SQL function result coercions.
Since commit913bbd8, check_sql_fn_retval() can either insert typecoercion steps in-line in the Query that produces the SQL function'sresults, 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, becausethe code tried to inject the new Query back into the query list it waspassed ... which is not the list that will be used for later processingwhen we execute the SQL function "normally" (without inlining it).So we ended up with no coercion happening at run-time, leading towrong 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 theseparate-Query path were checking either inline-able cases (whichaccidentally 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 coercionwasn't obvious. The fact that the cases that don't work weren'tallowed at all before v13 probably contributed to not noticing theproblem sooner, too.To fix, get rid of the separate "flat" list of Query nodes and insteadpass the real two-level list that is going to be used later. I choseto make the same change in check_sql_fn_statements(), although that hasno 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 tocallers outside functions.c. That's a bit scary to be doing in areleased branch, but so far as I can tell from a quick search,there are no outside callers of these functions (and they aresufficiently specific to our semantics for SQL-language functions thatit's not apparent why any extension would need to call them). In anycase, v13 already changed the API of check_sql_fn_retval() compared toprior 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 parentc5f42da commitc8ab970

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
@@ -913,8 +913,8 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
913913
(ParserSetupHook)sql_fn_parser_setup,
914914
pinfo,
915915
NULL);
916-
querytree_list=list_concat(querytree_list,
917-
querytree_sublist);
916+
querytree_list=lappend(querytree_list,
917+
querytree_sublist);
918918
}
919919

920920
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
@@ -609,7 +609,6 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
609609
SQLFunctionCachePtrfcache;
610610
List*raw_parsetree_list;
611611
List*queryTree_list;
612-
List*flat_query_list;
613612
List*resulttlist;
614613
ListCell*lc;
615614
Datumtmp;
@@ -689,13 +688,7 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
689688

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

707700
queryTree_list=NIL;
708-
flat_query_list=NIL;
709701
foreach(lc,raw_parsetree_list)
710702
{
711703
RawStmt*parsetree=lfirst_node(RawStmt,lc);
@@ -717,10 +709,12 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
717709
fcache->pinfo,
718710
NULL);
719711
queryTree_list=lappend(queryTree_list,queryTree_sublist);
720-
flat_query_list=list_concat(flat_query_list,queryTree_sublist);
721712
}
722713

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

725719
/*
726720
* Check that the function returns the type it claims to. Although in
@@ -740,7 +734,7 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
740734
* the rowtype column into multiple columns, since we have no way to
741735
* notify the caller that it should do that.)
742736
*/
743-
fcache->returnsTuple=check_sql_fn_retval(flat_query_list,
737+
fcache->returnsTuple=check_sql_fn_retval(queryTree_list,
744738
rettype,
745739
rettupdesc,
746740
false,
@@ -1510,51 +1504,63 @@ ShutdownSQLFunction(Datum arg)
15101504
* is not acceptable.
15111505
*/
15121506
void
1513-
check_sql_fn_statements(List*queryTreeList)
1507+
check_sql_fn_statements(List*queryTreeLists)
15141508
{
15151509
ListCell*lc;
15161510

1517-
foreach(lc,queryTreeList)
1511+
/* We are given a list of sublists of Queries */
1512+
foreach(lc,queryTreeLists)
15181513
{
1519-
Query*query=lfirst_node(Query,lc);
1514+
List*sublist=lfirst_node(List,lc);
1515+
ListCell*lc2;
15201516

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

15561561
/*
1557-
* check_sql_fn_retval() -- check return value of a list of sql parse trees.
1562+
* check_sql_fn_retval()
1563+
*Check return value of a list of lists of sql parse trees.
15581564
*
15591565
* The return value of a sql function is the value returned by the last
15601566
* canSetTag query in the function. We do some ad-hoc type checking and
@@ -1592,7 +1598,7 @@ check_sql_fn_statements(List *queryTreeList)
15921598
* function is defined to return VOID then *resultTargetList is set to NIL.
15931599
*/
15941600
bool
1595-
check_sql_fn_retval(List*queryTreeList,
1601+
check_sql_fn_retval(List*queryTreeLists,
15961602
Oidrettype,TupleDescrettupdesc,
15971603
boolinsertDroppedCols,
15981604
List**resultTargetList)
@@ -1619,20 +1625,30 @@ check_sql_fn_retval(List *queryTreeList,
16191625
return false;
16201626

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

1632-
if (q->canSetTag)
1643+
foreach(lc2,sublist)
16331644
{
1634-
parse=q;
1635-
parse_cell=lc;
1645+
Query*q=lfirst_node(Query,lc2);
1646+
1647+
if (q->canSetTag)
1648+
{
1649+
parse=q;
1650+
parse_cell=lc2;
1651+
}
16361652
}
16371653
}
16381654

‎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
gotofail;/* 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
externvoidsql_fn_parser_setup(structParseState*pstate,
3030
SQLFunctionParseInfoPtrpinfo);
3131

32-
externvoidcheck_sql_fn_statements(List*queryTreeList);
32+
externvoidcheck_sql_fn_statements(List*queryTreeLists);
3333

34-
externboolcheck_sql_fn_retval(List*queryTreeList,
34+
externboolcheck_sql_fn_retval(List*queryTreeLists,
3535
Oidrettype,TupleDescrettupdesc,
3636
boolinsertDroppedCols,
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
-- Check a couple of error cases while we're here
21132157
select * from testrngfunc() as t(f1 int8,f2 int8); -- fail, composite result
21142158
ERROR: a column definition list is redundant for a function returning a named composite type

‎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 replacefunctiontestrngfunc() returns setof rngfunc_typeas $$
633+
select1,2unionselect3,4order by1;
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
-- Check a couple of error cases while we're here
633644
select*from testrngfunc()as t(f1 int8,f2 int8);-- fail, composite result
634645
select*from pg_get_keywords()as t(f1 int8,f2 int8);-- fail, OUT params

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp