8000 Fix plpgsql's reporting of plan-time errors in possibly-simple expres… · danielcode/postgres@e09825a · GitHub
[go: up one dir, main page]
Skip to content

Commit e09825a

Browse files
tglsfdctglsfdc
committed
Fix plpgsql's reporting of plan-time errors in possibly-simple expressions.
exec_simple_check_plan and exec_eval_simple_expr attempted to call GetCachedPlan directly. This meant that if an error was thrown during planning, the resulting context traceback would not include the line normally 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 cached the plan and marked the expression as non-simple. The problem is easy to demonstrate in 9.2 and HEAD because planning of a cached plan doesn't occur at all until GetCachedPlan is done. In earlier versions, it could only be an issue if initial planning had succeeded, then a replan was forced (already somewhat improbable for a simple expression), and the replan attempt failed. Since the issue is mainly cosmetic in older branches 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 can affect the results of GET STACKED DIAGNOSTICS, as per a recent discussion on pgsql-novice. To fix, introduce a SPI function that wraps GetCachedPlan while installing the correct callback function. Use this instead of calling GetCachedPlan directly from plpgsql. Also introduce a wrapper function for extracting a SPI plan's CachedPlanSource list. This lets us stop including spi_priv.h in pl_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 parent e7105c5 commit e09825a

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
@@ -1131,6 +1131,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
11311131
Snapshot snapshot;
11321132
MemoryContext oldcontext;
11331133
Portal portal;
1134+
ErrorContextCallback spierrcontext;
11341135

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

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

1203+
/* Pop the error context stack */
1204+
error_context_stack = spierrcontext.previous;
1205+
11931206
if (!plan->saved)
11941207
{
11951208
/*
@@ -1551,6 +1564,65 @@ SPI_result_code_string(int code)
15511564
return buf;
15521565
}
15531566

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

15561628
/*

‎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
extern bool SPI_plan_is_valid(SPIPlanPtr plan);
104104
extern const char *SPI_result_code_string(int code);
105105

106+
extern List *SPI_plan_get_plan_sources(SPIPlanPtr plan);
107+
extern CachedPlan *SPI_plan_get_cached_plan(SPIPlanPtr plan);
108+
106109
extern HeapTuple SPI_copytuple(HeapTuple tuple);
107110
extern HeapTupleHeader SPI_returntuple(HeapTuple tuple, TupleDesc tupdesc);
108111
extern HeapTuple SPI_modifytuple(Relation rel, HeapTuple tuple, int natts,

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

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
#include "access/tupconvert.h"
2222
#include "catalog/pg_proc.h"
2323
#include "catalog/pg_type.h"
24-
#include "executor/spi_priv.h"
24+
#include "executor/spi.h"
2525
#include "funcapi.h"
2626
#include "miscadmin.h"
2727
#include "nodes/nodeFuncs.h"
@@ -3029,7 +3029,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
30293029

30303030
exec_prepare_plan(estate, expr, 0);
30313031
stmt->mod_stmt = false;
3032-
foreach(l, expr->plan->plancache_list)
3032+
foreach(l, SPI_plan_get_plan_sources(expr->plan))
30333033
{
30343034
CachedPlanSource *plansource = (CachedPlanSource *) lfirst(l);
30353035
ListCell *l2;
@@ -4846,10 +4846,11 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
48464846
*
48474847
* It is possible though unlikely for a simple expression to become non-simple
48484848
* (consider for example redefining a trivial view). We must handle that for
4849-
* correctness; fortunately it's normally inexpensive to do GetCachedPlan on a
4850-
* simple expression. We do not consider the other direction (non-simple
4851-
* expression becoming simple) because we'll still give correct results if
4852-
* that happens, and it's unlikely to be worth the cycles to check.
4849+
* correctness; fortunately it's normally inexpensive to call
4850+
* SPI_plan_get_cached_plan for a simple expression. We do not consider the
4851+
* other direction (non-simple expression becoming simple) because we'll still
4852+
* give correct results if that happens, and it's unlikely to be worth the
4853+
* cycles to check.
48534854
*
48544855
* Note: if pass-by-reference, the result is in the eval_econtext's
48554856
* temporary memory context. It will be freed when exec_eval_cleanup
@@ -4865,7 +4866,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
48654866
{
48664867
ExprContext *econtext = estate->eval_econtext;
48674868
LocalTransactionId curlxid = MyProc->lxid;
4868-
CachedPlanSource *plansource;
48694869
CachedPlan *cplan;
48704870
ParamListInfo paramLI;
48714871
PLpgSQL_expr *save_cur_expr;
@@ -4885,16 +4885,16 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
48854885

48864886
/*
48874887
* Revalidate cached plan, so that we will notice if it became stale. (We
4888-
* need to hold a refcount while using the plan, anyway.) Note that even
4889-
* if replanning occurs, the length of plancache_list can't change, since
4890-
* it is a property of the raw parsetree generated from the query text.
4888+
* need to hold a refcount while using the plan, anyway.)
48914889
*/
4892-
Assert(list_length(expr->plan->plancache_list) == 1);
4893-
plansource = (CachedPlanSource *) linitial(expr->plan->plancache_list);
4890+
cplan = SPI_plan_get_cached_plan(expr->plan);
48944891

4895-
/* Get the generic plan for the query */
4896-
cplan = GetCachedPlan(plansource, NULL, true);
4897-
Assert(cplan == plansource->gplan);
4892+
/*
4893+
* We can't get a failure here, because the number of CachedPlanSources in
4894+
* the SPI plan can't change from what exec_simple_check_plan saw; it's a
4895+
* property of the raw parsetree generated from the query text.
4896+
*/
4897+
Assert(cplan != NULL);
48984898

48994899
if (cplan->generation != expr->expr_simple_generation)
49004900
{
@@ -5702,6 +5702,7 @@ exec_simple_check_node(Node *node)
57025702
static void
57035703
exec_simple_check_plan(PLpgSQL_expr *expr)
57045704
{
5705+
List *plansources;
57055706
CachedPlanSource *plansource;
57065707
Query *query;
57075708
CachedPlan *cplan;
@@ -5717,9 +5718,10 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
57175718
/*
57185719
* We can only test queries that resulted in exactly one CachedPlanSource
57195720
*/
5720-
if (list_length(expr->plan->plancache_list) != 1)
5721+
plansources = SPI_plan_get_plan_sources(expr->plan);
5722+
if (list_length(plansources) != 1)
57215723
return;
5722-
plansource = (CachedPlanSource *) linitial(expr->plan->plancache_list);
5724+
plansource = (CachedPlanSource *) linitial(plansources);
57235725

57245726
/*
57255727
* Do some checking on the analyzed-and-rewritten form of the query. These
@@ -5777,8 +5779,10 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
57775779
*/
57785780

57795781
/* Get the generic plan for the query */
5780-
cplan = GetCachedPlan(plansource, NULL, true);
5781-
Assert(cplan == plansource->gplan);
5782+
cplan = SPI_plan_get_cached_plan(expr->plan);
5783+
5784+
/* Can't fail, because we checked for a single CachedPlanSource above */
5785+
Assert(cplan != NULL);
57825786

57835787
/* Share the remaining work with recheck code path */
57845788
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
@@ -4258,6 +4258,21 @@ select error2('public.stuffs');
42584258
rollback;
42594259
drop function error2(p_name_table text);
42604260
drop function error1(text);
4261+
-- Test for consistent reporting of error context
4262+
create function fail() returns int language plpgsql as $$
4263+
begin
4264+
return 1/0;
4265+
end
4266+
$$;
4267+
select fail();
4268+
ERROR: division by zero
4269+
CONTEXT: SQL statement "SELECT 1/0"
4270+
PL/pgSQL function fail() line 3 at RETURN
4271+
select fail();
4272+
ERROR: division by zero
4273+
CONTEXT: SQL statement "SELECT 1/0"
4274+
PL/pgSQL function fail() line 3 at RETURN
4275+
drop function fail();
42614276
-- Test handling of string literals.
42624277
set standard_conforming_strings = off;
42634278
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
@@ -3425,6 +3425,19 @@ rollback;
34253425
drop function error2(p_name_table text);
34263426
drop function error1(text);
34273427

3428+
-- Test for consistent reporting of error context
3429+
3430+
create function fail() returns int language plpgsql as $$
3431+
begin
3432+
return 1/0;
3433+
end
3434+
$$;
3435+
3436+
select fail();
3437+
select fail();
3438+
3439+
drop function fail();
3440+
34283441
-- Test handling of string literals.
34293442

34303443
set standard_conforming_strings = off;

0 commit comments

Comments
 (0)
0