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

Commit0900ac2

Browse files
committed
Fix plpgsql's reporting of plan-time errors in possibly-simple expressions.
exec_simple_check_plan and exec_eval_simple_expr attempted to callGetCachedPlan directly. This meant that if an error was thrown duringplanning, the resulting context traceback would not include the linenormally contributed by _SPI_error_callback. This is already inconsistent,but just to be really odd, a re-execution of the very same expression*would* show the additional context line, because we'd already have cachedthe plan and marked the expression as non-simple.The problem is easy to demonstrate in 9.2 and HEAD because planning of acached plan doesn't occur at all until GetCachedPlan is done. In earlierversions, it could only be an issue if initial planning had succeeded, thena replan was forced (already somewhat improbable for a simple expression),and the replan attempt failed. Since the issue is mainly cosmetic in olderbranches anyway, it doesn't seem worth the risk of trying to fix it there.It is worth fixing in 9.2 since the instability of the context printout canaffect the results of GET STACKED DIAGNOSTICS, as per a recent discussionon pgsql-novice.To fix, introduce a SPI function that wraps GetCachedPlan while installingthe correct callback function. Use this instead of calling GetCachedPlandirectly from plpgsql.Also introduce a wrapper function for extracting a SPI plan'sCachedPlanSource list. This lets us stop including spi_priv.h inpl_exec.c, which was never a very good idea from a modularity standpoint.In passing, fix a similar inconsistency that could occur in SPI_cursor_open,which was also calling GetCachedPlan without setting up a context callback.
1 parent670a6c7 commit0900ac2

File tree

5 files changed

+126
-19
lines changed

5 files changed

+126
-19
lines changed

‎src/backend/executor/spi.c

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
11321132
Snapshotsnapshot;
11331133
MemoryContextoldcontext;
11341134
Portalportal;
1135+
ErrorContextCallbackspierrcontext;
11351136

11361137
/*
11371138
* Check that the plan is something the Portal code will special-case as
@@ -1181,6 +1182,15 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
11811182
query_string=MemoryContextStrdup(PortalGetHeapMemory(portal),
11821183
plansource->query_string);
11831184

1185+
/*
1186+
* Setup error traceback support for ereport(), in case GetCachedPlan
1187+
* throws an error.
1188+
*/
1189+
spierrcontext.callback=_SPI_error_callback;
1190+
spierrcontext.arg= (void*)plansource->query_string;
1191+
spierrcontext.previous=error_context_stack;
1192+
error_context_stack=&spierrcontext;
1193+
11841194
/*
11851195
* Note: for a saved plan, we mustn't have any failure occur between
11861196
* GetCachedPlan and PortalDefineQuery; that would result in leaking our
@@ -1191,6 +1201,9 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
11911201
cplan=GetCachedPlan(plansource,paramLI, false);
11921202
stmt_list=cplan->stmt_list;
11931203

1204+
/* Pop the error context stack */
1205+
error_context_stack=spierrcontext.previous;
1206+
11941207
if (!plan->saved)
11951208
{
11961209
/*
@@ -1552,6 +1565,65 @@ SPI_result_code_string(int code)
15521565
returnbuf;
15531566
}
15541567

1568+
/*
1569+
* SPI_plan_get_plan_sources --- get a SPI plan's underlying list of
1570+
* CachedPlanSources.
1571+
*
1572+
* This is exported so that pl/pgsql can use it (this beats letting pl/pgsql
1573+
* look directly into the SPIPlan for itself). It's not documented in
1574+
* spi.sgml because we'd just as soon not have too many places using this.
1575+
*/
1576+
List*
1577+
SPI_plan_get_plan_sources(SPIPlanPtrplan)
1578+
{
1579+
Assert(plan->magic==_SPI_PLAN_MAGIC);
1580+
returnplan->plancache_list;
1581+
}
1582+
1583+
/*
1584+
* SPI_plan_get_cached_plan --- get a SPI plan's generic CachedPlan,
1585+
* if the SPI plan contains exactly one CachedPlanSource. If not,
1586+
* return NULL. Caller is responsible for doing ReleaseCachedPlan().
1587+
*
1588+
* This is exported so that pl/pgsql can use it (this beats letting pl/pgsql
1589+
* look directly into the SPIPlan for itself). It's not documented in
1590+
* spi.sgml because we'd just as soon not have too many places using this.
1591+
*/
1592+
CachedPlan*
1593+
SPI_plan_get_cached_plan(SPIPlanPtrplan)
1594+
{
1595+
CachedPlanSource*plansource;
1596+
CachedPlan*cplan;
1597+
ErrorContextCallbackspierrcontext;
1598+
1599+
Assert(plan->magic==_SPI_PLAN_MAGIC);
1600+
1601+
/* Can't support one-shot plans here */
1602+
if (plan->oneshot)
1603+
returnNULL;
1604+
1605+
/* Must have exactly one CachedPlanSource */
1606+
if (list_length(plan->plancache_list)!=1)
1607+
returnNULL;
1608+
plansource= (CachedPlanSource*)linitial(plan->plancache_list);
1609+
1610+
/* Setup error traceback support for ereport() */
1611+
spierrcontext.callback=_SPI_error_callback;
1612+
spierrcontext.arg= (void*)plansource->query_string;
1613+
spierrcontext.previous=error_context_stack;
1614+
error_context_stack=&spierrcontext;
1615+
1616+
/* Get the generic plan for the query */
1617+
cplan=GetCachedPlan(plansource,NULL,plan->saved);
1618+
Assert(cplan==plansource->gplan);
1619+
1620+
/* Pop the error context stack */
1621+
error_context_stack=spierrcontext.previous;
1622+
1623+
returncplan;
1624+
}
1625+
1626+
15551627
/* =================== private functions =================== */
15561628

15571629
/*

‎src/include/executor/spi.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,9 @@ extern bool SPI_is_cursor_plan(SPIPlanPtr plan);
103103
externboolSPI_plan_is_valid(SPIPlanPtrplan);
104104
externconstchar*SPI_result_code_string(intcode);
105105

106+
externList*SPI_plan_get_plan_sources(SPIPlanPtrplan);
107+
externCachedPlan*SPI_plan_get_cached_plan(SPIPlanPtrplan);
108+
106109
externHeapTupleSPI_copytuple(HeapTupletuple);
107110
externHeapTupleHeaderSPI_returntuple(HeapTupletuple,TupleDesctupdesc);
108111
externHeapTupleSPI_modifytuple(Relationrel,HeapTupletuple,intnatts,

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
#include"access/tupconvert.h"
2323
#include"catalog/pg_proc.h"
2424
#include"catalog/pg_type.h"
25-
#include"executor/spi_priv.h"
25+
#include"executor/spi.h"
2626
#include"funcapi.h"
2727
#include"miscadmin.h"
2828
#include"nodes/nodeFuncs.h"
@@ -3174,7 +3174,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
31743174

31753175
exec_prepare_plan(estate,expr,0);
31763176
stmt->mod_stmt= false;
3177-
foreach(l,expr->plan->plancache_list)
3177+
foreach(l,SPI_plan_get_plan_sources(expr->plan))
31783178
{
31793179
CachedPlanSource*plansource= (CachedPlanSource*)lfirst(l);
31803180
ListCell*l2;
@@ -4954,10 +4954,11 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
49544954
*
49554955
* It is possible though unlikely for a simple expression to become non-simple
49564956
* (consider for example redefining a trivial view). We must handle that for
4957-
* correctness; fortunately it's normally inexpensive to do GetCachedPlan on a
4958-
* simple expression. We do not consider the other direction (non-simple
4959-
* expression becoming simple) because we'll still give correct results if
4960-
* that happens, and it's unlikely to be worth the cycles to check.
4957+
* correctness; fortunately it's normally inexpensive to call
4958+
* SPI_plan_get_cached_plan for a simple expression. We do not consider the
4959+
* other direction (non-simple expression becoming simple) because we'll still
4960+
* give correct results if that happens, and it's unlikely to be worth the
4961+
* cycles to check.
49614962
*
49624963
* Note: if pass-by-reference, the result is in the eval_econtext's
49634964
* temporary memory context. It will be freed when exec_eval_cleanup
@@ -4973,7 +4974,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
49734974
{
49744975
ExprContext*econtext=estate->eval_econtext;
49754976
LocalTransactionIdcurlxid=MyProc->lxid;
4976-
CachedPlanSource*plansource;
49774977
CachedPlan*cplan;
49784978
ParamListInfoparamLI;
49794979
PLpgSQL_expr*save_cur_expr;
@@ -4993,16 +4993,16 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
49934993

49944994
/*
49954995
* Revalidate cached plan, so that we will notice if it became stale. (We
4996-
* need to hold a refcount while using the plan, anyway.) Note that even
4997-
* if replanning occurs, the length of plancache_list can't change, since
4998-
* it is a property of the raw parsetree generated from the query text.
4996+
* need to hold a refcount while using the plan, anyway.)
49994997
*/
5000-
Assert(list_length(expr->plan->plancache_list)==1);
5001-
plansource= (CachedPlanSource*)linitial(expr->plan->plancache_list);
4998+
cplan=SPI_plan_get_cached_plan(expr->plan);
50024999

5003-
/* Get the generic plan for the query */
5004-
cplan=GetCachedPlan(plansource,NULL, true);
5005-
Assert(cplan==plansource->gplan);
5000+
/*
5001+
* We can't get a failure here, because the number of CachedPlanSources in
5002+
* the SPI plan can't change from what exec_simple_check_plan saw; it's a
5003+
* property of the raw parsetree generated from the query text.
5004+
*/
5005+
Assert(cplan!=NULL);
50065006

50075007
if (cplan->generation!=expr->expr_simple_generation)
50085008
{
@@ -5893,6 +5893,7 @@ exec_simple_check_node(Node *node)
58935893
staticvoid
58945894
exec_simple_check_plan(PLpgSQL_expr*expr)
58955895
{
5896+
List*plansources;
58965897
CachedPlanSource*plansource;
58975898
Query*query;
58985899
CachedPlan*cplan;
@@ -5908,9 +5909,10 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
59085909
/*
59095910
* We can only test queries that resulted in exactly one CachedPlanSource
59105911
*/
5911-
if (list_length(expr->plan->plancache_list)!=1)
5912+
plansources=SPI_plan_get_plan_sources(expr->plan);
5913+
if (list_length(plansources)!=1)
59125914
return;
5913-
plansource= (CachedPlanSource*)linitial(expr->plan->plancache_list);
5915+
plansource= (CachedPlanSource*)linitial(plansources);
59145916

59155917
/*
59165918
* Do some checking on the analyzed-and-rewritten form of the query. These
@@ -5968,8 +5970,10 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
59685970
*/
59695971

59705972
/* Get the generic plan for the query */
5971-
cplan=GetCachedPlan(plansource,NULL, true);
5972-
Assert(cplan==plansource->gplan);
5973+
cplan=SPI_plan_get_cached_plan(expr->plan);
5974+
5975+
/* Can't fail, because we checked for a single CachedPlanSource above */
5976+
Assert(cplan!=NULL);
59735977

59745978
/* Share the remaining work with recheck code path */
59755979
exec_simple_recheck_plan(expr,cplan);

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4387,6 +4387,21 @@ select error2('public.stuffs');
43874387
rollback;
43884388
drop function error2(p_name_table text);
43894389
drop function error1(text);
4390+
-- Test for consistent reporting of error context
4391+
create function fail() returns int language plpgsql as $$
4392+
begin
4393+
return 1/0;
4394+
end
4395+
$$;
4396+
select fail();
4397+
ERROR: division by zero
4398+
CONTEXT: SQL statement "SELECT 1/0"
4399+
PL/pgSQL function fail() line 3 at RETURN
4400+
select fail();
4401+
ERROR: division by zero
4402+
CONTEXT: SQL statement "SELECT 1/0"
4403+
PL/pgSQL function fail() line 3 at RETURN
4404+
drop function fail();
43904405
-- Test handling of string literals.
43914406
set standard_conforming_strings = off;
43924407
create or replace function strtest() returns text as $$

‎src/test/regress/sql/plpgsql.sql

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3537,6 +3537,19 @@ rollback;
35373537
dropfunction error2(p_name_tabletext);
35383538
dropfunction error1(text);
35393539

3540+
-- Test for consistent reporting of error context
3541+
3542+
createfunctionfail() returnsint language plpgsqlas $$
3543+
begin
3544+
return1/0;
3545+
end
3546+
$$;
3547+
3548+
select fail();
3549+
select fail();
3550+
3551+
dropfunction fail();
3552+
35403553
-- Test handling of string literals.
35413554

35423555
set standard_conforming_strings= off;

0 commit comments

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