Thread: Better localization of errors in plpgsql variable initialization

While preparing commit a2a731d6c, I chanced to notice what seemed
like an off-by-one error in the line number reported for an error
occurring while we initialize a plpgsql variable.  For instance

regression=# do $$ declare x int := 1/0;
regression$# begin end $$;
ERROR:  division by zero
CONTEXT:  SQL expression "1/0"
PL/pgSQL function inline_code_block line 2 during statement block local variable initialization

After tracking it down, it's not that.  It's that we're pointing
at the BEGIN keyword, because that's what determines the lineno
associated with the PLpgSQL_stmt_block struct.

This seems like it could be quite confusing in a block with many
variables, so here's a proposed patch that improves the reported
line number to match the specific variable.

The only downside that I can see here is the one additional store
per variable to set err_var; but it's pretty hard to believe that
that'd be measurable in context.

Any objections?

            regards, tom lane

diff --git a/src/pl/plpgsql/src/expected/plpgsql_domain.out b/src/pl/plpgsql/src/expected/plpgsql_domain.out
index efc877cdd1..516c2b9e08 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_domain.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_domain.out
@@ -33,7 +33,7 @@ SELECT * FROM test_assign_booltrue(true, true);

 SELECT * FROM test_assign_booltrue(false, true);
 ERROR:  value for domain booltrue violates check constraint "booltrue_check"
-CONTEXT:  PL/pgSQL function test_assign_booltrue(boolean,boolean) line 3 during statement block local variable
initialization
+CONTEXT:  PL/pgSQL function test_assign_booltrue(boolean,boolean) line 2 during statement block local variable
initialization
 SELECT * FROM test_assign_booltrue(true, false);
 ERROR:  value for domain booltrue violates check constraint "booltrue_check"
 CONTEXT:  PL/pgSQL function test_assign_booltrue(boolean,boolean) line 4 at assignment
@@ -76,7 +76,7 @@ ERROR:  value for domain uint2 violates check constraint "uint2_check"
 CONTEXT:  PL/pgSQL function test_assign_uint2(integer,integer) line 4 at assignment
 SELECT * FROM test_assign_uint2(-100, 50);
 ERROR:  value for domain uint2 violates check constraint "uint2_check"
-CONTEXT:  PL/pgSQL function test_assign_uint2(integer,integer) line 3 during statement block local variable
initialization
+CONTEXT:  PL/pgSQL function test_assign_uint2(integer,integer) line 2 during statement block local variable
initialization
 SELECT * FROM test_assign_uint2(null, 1);
  test_assign_uint2
 -------------------
@@ -115,7 +115,7 @@ SELECT * FROM test_assign_nnint(10, 20);

 SELECT * FROM test_assign_nnint(null, 20);
 ERROR:  domain nnint does not allow null values
-CONTEXT:  PL/pgSQL function test_assign_nnint(integer,integer) line 3 during statement block local variable
initialization
+CONTEXT:  PL/pgSQL function test_assign_nnint(integer,integer) line 2 during statement block local variable
initialization
 SELECT * FROM test_assign_nnint(10, null);
 ERROR:  domain nnint does not allow null values
 CONTEXT:  PL/pgSQL function test_assign_nnint(integer,integer) line 4 at assignment
@@ -168,7 +168,7 @@ ERROR:  value for domain ordered_pair_domain violates check constraint "ordered_
 CONTEXT:  PL/pgSQL function test_assign_ordered_pair_domain(integer,integer,integer) line 4 at assignment
 SELECT * FROM test_assign_ordered_pair_domain(2,1,3);
 ERROR:  value for domain ordered_pair_domain violates check constraint "ordered_pair_domain_check"
-CONTEXT:  PL/pgSQL function test_assign_ordered_pair_domain(integer,integer,integer) line 3 during statement block
localvariable initialization 
+CONTEXT:  PL/pgSQL function test_assign_ordered_pair_domain(integer,integer,integer) line 2 during statement block
localvariable initialization 
 --
 -- Arrays of domains
 --
@@ -276,7 +276,7 @@ ERROR:  domain nnint does not allow null values
 CONTEXT:  PL/pgSQL function test_assign_nnint_container(integer,integer,integer) line 4 at assignment
 SELECT * FROM test_assign_nnint_container(1,null,3);
 ERROR:  domain nnint does not allow null values
-CONTEXT:  PL/pgSQL function test_assign_nnint_container(integer,integer,integer) line 3 during statement block local
variableinitialization 
+CONTEXT:  PL/pgSQL function test_assign_nnint_container(integer,integer,integer) line 2 during statement block local
variableinitialization 
 -- Since core system allows this:
 SELECT null::nnint_container;
  nnint_container
@@ -356,7 +356,7 @@ ERROR:  value for domain ordered_named_pair violates check constraint "ordered_n
 CONTEXT:  PL/pgSQL function test_assign_ordered_named_pair(integer,integer,integer) line 4 at assignment
 SELECT * FROM test_assign_ordered_named_pair(2,1,3);
 ERROR:  value for domain ordered_named_pair violates check constraint "ordered_named_pair_check"
-CONTEXT:  PL/pgSQL function test_assign_ordered_named_pair(integer,integer,integer) line 3 during statement block
localvariable initialization 
+CONTEXT:  PL/pgSQL function test_assign_ordered_named_pair(integer,integer,integer) line 2 during statement block
localvariable initialization 
 CREATE FUNCTION build_ordered_named_pairs(i int, j int) RETURNS ordered_named_pair[] AS $$
 begin
 return array[row(i, j), row(i, j+1)];
@@ -388,7 +388,7 @@ SELECT * FROM test_assign_ordered_named_pairs(1,2,3);

 SELECT * FROM test_assign_ordered_named_pairs(2,1,3);
 ERROR:  value for domain ordered_named_pair violates check constraint "ordered_named_pair_check"
-CONTEXT:  PL/pgSQL function test_assign_ordered_named_pairs(integer,integer,integer) line 3 during statement block
localvariable initialization 
+CONTEXT:  PL/pgSQL function test_assign_ordered_named_pairs(integer,integer,integer) line 2 during statement block
localvariable initialization 
 SELECT * FROM test_assign_ordered_named_pairs(1,2,0);  -- should fail someday
  test_assign_ordered_named_pairs
 ---------------------------------
diff --git a/src/pl/plpgsql/src/expected/plpgsql_varprops.out b/src/pl/plpgsql/src/expected/plpgsql_varprops.out
index 3801dccc95..25115a02bd 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_varprops.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_varprops.out
@@ -77,7 +77,7 @@ begin
 end$$;
 ERROR:  division by zero
 CONTEXT:  SQL expression "1/0"
-PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x bigint[] := array[1,3,5];
 begin
@@ -120,7 +120,7 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  null value cannot be assigned to variable "x" declared NOT NULL
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x record not null;  -- fail
 begin
@@ -147,7 +147,7 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  null value cannot be assigned to variable "x" declared NOT NULL
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x var_record not null;  -- fail
 begin
@@ -174,7 +174,7 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  null value cannot be assigned to variable "x" declared NOT NULL
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 -- Check that variables are reinitialized on block re-entry.
 do $$
 begin
@@ -216,14 +216,14 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  domain int_nn does not allow null values
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x int_nn := null;  -- fail
 begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  domain int_nn does not allow null values
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x int_nn := 42;
 begin
@@ -239,14 +239,14 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  domain var_record_nn does not allow null values
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x var_record_nn := null;  -- fail
 begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  domain var_record_nn does not allow null values
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x var_record_nn := row(1,2);
 begin
@@ -263,21 +263,21 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  value for domain var_record_colnn violates check constraint "var_record_colnn_check"
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x var_record_colnn := null;  -- fail
 begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  value for domain var_record_colnn violates check constraint "var_record_colnn_check"
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x var_record_colnn := row(1,null);  -- fail
 begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  value for domain var_record_colnn violates check constraint "var_record_colnn_check"
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x var_record_colnn := row(1,2);
 begin
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 0e1cfa3df6..6dbfdb7be0 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1214,6 +1214,20 @@ static void
 plpgsql_exec_error_callback(void *arg)
 {
     PLpgSQL_execstate *estate = (PLpgSQL_execstate *) arg;
+    int            err_lineno;
+
+    /*
+     * If err_var is set, report the variable's declaration line number.
+     * Otherwise, if err_stmt is set, report the err_stmt's line number.  When
+     * err_stmt is not set, we're in function entry/exit, or some such place
+     * not attached to a specific line number.
+     */
+    if (estate->err_var != NULL)
+        err_lineno = estate->err_var->lineno;
+    else if (estate->err_stmt != NULL)
+        err_lineno = estate->err_stmt->lineno;
+    else
+        err_lineno = 0;

     if (estate->err_text != NULL)
     {
@@ -1222,13 +1236,8 @@ plpgsql_exec_error_callback(void *arg)
          * actually need it.  Therefore, places that set up err_text should
          * use gettext_noop() to ensure the strings get recorded in the
          * message dictionary.
-         *
-         * If both err_text and err_stmt are set, use the err_text as
-         * description, but report the err_stmt's line number.  When err_stmt
-         * is not set, we're in function entry/exit, or some such place not
-         * attached to a specific line number.
          */
-        if (estate->err_stmt != NULL)
+        if (err_lineno > 0)
         {
             /*
              * translator: last %s is a phrase such as "during statement block
@@ -1236,7 +1245,7 @@ plpgsql_exec_error_callback(void *arg)
              */
             errcontext("PL/pgSQL function %s line %d %s",
                        estate->func->fn_signature,
-                       estate->err_stmt->lineno,
+                       err_lineno,
                        _(estate->err_text));
         }
         else
@@ -1250,12 +1259,12 @@ plpgsql_exec_error_callback(void *arg)
                        _(estate->err_text));
         }
     }
-    else if (estate->err_stmt != NULL)
+    else if (estate->err_stmt != NULL && err_lineno > 0)
     {
         /* translator: last %s is a plpgsql statement type name */
         errcontext("PL/pgSQL function %s line %d at %s",
                    estate->func->fn_signature,
-                   estate->err_stmt->lineno,
+                   err_lineno,
                    plpgsql_stmt_typename(estate->err_stmt));
     }
     else
@@ -1643,7 +1652,12 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
          * Note that we currently don't support promise datums within blocks,
          * only at a function's outermost scope, so we needn't handle those
          * here.
+         *
+         * Since RECFIELD isn't a supported case either, it's okay to cast the
+         * PLpgSQL_datum to PLpgSQL_variable.
          */
+        estate->err_var = (PLpgSQL_variable *) datum;
+
         switch (datum->dtype)
         {
             case PLPGSQL_DTYPE_VAR:
@@ -1717,6 +1731,8 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
         }
     }

+    estate->err_var = NULL;
+
     if (block->exceptions)
     {
         /*
@@ -4041,6 +4057,7 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
     estate->eval_econtext = NULL;

     estate->err_stmt = NULL;
+    estate->err_var = NULL;
     estate->err_text = NULL;

     estate->plugin_info = NULL;
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index ebd3a5d3c8..3936866bb7 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1088,6 +1088,7 @@ typedef struct PLpgSQL_execstate

     /* status information for error context reporting */
     PLpgSQL_stmt *err_stmt;        /* current stmt */
+    PLpgSQL_variable *err_var;    /* current variable, if in a DECLARE section */
     const char *err_text;        /* additional state info */

     void       *plugin_info;    /* reserved for use by optional plugin */
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index a04bd00ac6..8faf46d404 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -847,7 +847,7 @@ begin
 end$$ language plpgsql;
 select doubledecrement(3); -- fail because of implicit null assignment
 ERROR:  domain pos_int does not allow null values
-CONTEXT:  PL/pgSQL function doubledecrement(pos_int) line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function doubledecrement(pos_int) line 2 during statement block local variable initialization
 create or replace function doubledecrement(p1 pos_int) returns pos_int as $$
 declare v pos_int := 0;
 begin
@@ -855,7 +855,7 @@ begin
 end$$ language plpgsql;
 select doubledecrement(3); -- fail at initialization assignment
 ERROR:  value for domain pos_int violates check constraint "pos_int_check"
-CONTEXT:  PL/pgSQL function doubledecrement(pos_int) line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function doubledecrement(pos_int) line 2 during statement block local variable initialization
 create or replace function doubledecrement(p1 pos_int) returns pos_int as $$
 declare v pos_int := 1;
 begin
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index ae6fc824b6..33eb5e54b9 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -4648,7 +4648,7 @@ ERROR:  column "x" does not exist
 LINE 1: x + 1
         ^
 QUERY:  x + 1
-CONTEXT:  PL/pgSQL function inline_code_block line 3 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare y int := x + 1;  -- error
         x int := 42;
@@ -4660,7 +4660,7 @@ ERROR:  column "x" does not exist
 LINE 1: x + 1
         ^
 QUERY:  x + 1
-CONTEXT:  PL/pgSQL function inline_code_block line 4 during statement block local variable initialization
+CONTEXT:  PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x int := 42;
         y int := x + 1;