Re: Mention column name in error messages - Mailing list pgsql-hackers

I wrote:
> ... I wonder whether we should
> not try to move in the direction of expanding the set of errors that we
> can provide error cursor info for.  One aspect of that would be making
> sure that the text of the current statement is always available at
> runtime.  I believe we do always have it somewhere now, but I'm not sure
> that it's passed through to the executor in any convenient way.  The
> other aspect would then be to provide errlocation information based on
> the expression node that we're currently executing.  That seems do-able
> probably.  The overhead would likely consist of storing a pointer to the
> current node someplace where an error context callback could find it.
> Which is not zero cost, but I think it wouldn't be awful.

Just to demonstrate what I'm talking about, I made a really dirty
proof-of-concept patch, attached.  With this, you get results like

regression=# create table foo(bar varchar(4), baz varchar(2));
CREATE TABLE
regression=# insert into foo values ('foo2!', 'ok');
ERROR:  value too long for type character varying(4)
LINE 1: insert into foo values ('foo2!', 'ok');
                                ^

which was the original complaint, and it also works for run-time cases:

regression=# insert into foo values (random()::text, 'ok');
ERROR:  value too long for type character varying(4)
LINE 1: insert into foo values (random()::text, 'ok');
                                ^

If that seems like a satisfactory behavior, somebody could push forward
with turning this into a committable patch.  There's a lot to do:

* I just used debug_query_string as the statement text source, which means
it only works correctly for errors in directly client-issued statements.
We'd need some work on appropriately passing the correct text down to
execution.  (It would be a good idea to decouple elog.c from
debug_query_string while at it, I think.)

* I didn't do anything about nested execution contexts.  You'd want only
the innermost one to set the cursor pointer, but as the code stands
I think the outermost one would win.

* Some attention needs to be paid to factorizing the support nicely
--- multiple copies of the callback isn't good, and we might end up
with many copies of the setup boilerplate.  Maybe the parser's
pcb_error_callback infrastructure would be a useful prototype.

* I only bothered to hook in execution of function/operator nodes.
That's probably a 90% solution already, but it's not meant to be
the final version.

Likely, doing anything about the last point should wait on Andres' work
with linearizing expression execution.  As this patch stands, you have to
think carefully about where within an ExecEvalFoo function is the right
place to set cur_expr, since it shouldn't get set till after control
returns from subsidiary nodes.  But those recursive calls would go away.

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 743e7d6..81aa0b8 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 44,49 ****
--- 44,50 ----
  #include "executor/execdebug.h"
  #include "executor/nodeSubplan.h"
  #include "funcapi.h"
+ #include "mb/pg_wchar.h"
  #include "miscadmin.h"
  #include "nodes/makefuncs.h"
  #include "nodes/nodeFuncs.h"
***************
*** 51,56 ****
--- 52,58 ----
  #include "parser/parse_coerce.h"
  #include "parser/parsetree.h"
  #include "pgstat.h"
+ #include "tcop/tcopprot.h"
  #include "utils/acl.h"
  #include "utils/builtins.h"
  #include "utils/date.h"
*************** restart:
*** 1772,1777 ****
--- 1774,1782 ----
          fcache->setArgsValid = false;
      }

+     /* Treat function as active beginning here */
+     econtext->cur_expr = (ExprState *) fcache;
+
      /*
       * Now call the function, passing the evaluated parameter values.
       */
*************** restart:
*** 1969,1974 ****
--- 1974,1980 ----
                  if (fcinfo->argnull[i])
                  {
                      *isNull = true;
+                     econtext->cur_expr = NULL;
                      return (Datum) 0;
                  }
              }
*************** restart:
*** 1983,1988 ****
--- 1989,1996 ----
          pgstat_end_function_usage(&fcusage, true);
      }

+     econtext->cur_expr = NULL;
+
      return result;
  }

*************** ExecMakeFunctionResultNoSets(FuncExprSta
*** 2040,2045 ****
--- 2048,2056 ----
          }
      }

+     /* Treat function as active beginning here */
+     econtext->cur_expr = (ExprState *) fcache;
+
      pgstat_init_function_usage(fcinfo, &fcusage);

      fcinfo->isnull = false;
*************** ExecMakeFunctionResultNoSets(FuncExprSta
*** 2048,2053 ****
--- 2059,2066 ----

      pgstat_end_function_usage(&fcusage, true);

+     econtext->cur_expr = NULL;
+
      return result;
  }

*************** ExecPrepareExpr(Expr *node, EState *esta
*** 5309,5314 ****
--- 5322,5350 ----
   * ----------------------------------------------------------------
   */

+ static void
+ ExprErrorCallback(void *arg)
+ {
+     ExprContext *econtext = (ExprContext *) arg;
+
+     if (econtext->cur_expr)
+     {
+         int            location = exprLocation((Node *) econtext->cur_expr->expr);
+         int            pos;
+
+         /* No-op if location was not provided */
+         if (location < 0)
+             return;
+         /* Can't do anything if source text is not available */
+         if (debug_query_string == NULL)
+             return;
+         /* Convert offset to character number */
+         pos = pg_mbstrlen_with_len(debug_query_string, location) + 1;
+         /* And pass it to the ereport mechanism */
+         errposition(pos);
+     }
+ }
+
  /* ----------------------------------------------------------------
   *        ExecQual
   *
*************** bool
*** 5341,5346 ****
--- 5377,5383 ----
  ExecQual(List *qual, ExprContext *econtext, bool resultForNull)
  {
      bool        result;
+     ErrorContextCallback errcallback;
      MemoryContext oldContext;
      ListCell   *l;

*************** ExecQual(List *qual, ExprContext *econte
*** 5351,5356 ****
--- 5388,5400 ----
      EV_nodeDisplay(qual);
      EV_printf("\n");

+     /* Setup error traceback support for ereport() */
+     econtext->cur_expr = NULL;
+     errcallback.callback = ExprErrorCallback;
+     errcallback.arg = (void *) econtext;
+     errcallback.previous = error_context_stack;
+     error_context_stack = &errcallback;
+
      /*
       * Run in short-lived per-tuple context while computing expressions.
       */
*************** ExecQual(List *qual, ExprContext *econte
*** 5398,5403 ****
--- 5442,5449 ----

      MemoryContextSwitchTo(oldContext);

+     error_context_stack = errcallback.previous;
+
      return result;
  }

*************** ExecTargetList(List *targetlist,
*** 5463,5472 ****
--- 5509,5526 ----
                 ExprDoneCond *isDone)
  {
      Form_pg_attribute *att = tupdesc->attrs;
+     ErrorContextCallback errcallback;
      MemoryContext oldContext;
      ListCell   *tl;
      bool        haveDoneSets;

+     /* Setup error traceback support for ereport() */
+     econtext->cur_expr = NULL;
+     errcallback.callback = ExprErrorCallback;
+     errcallback.arg = (void *) econtext;
+     errcallback.previous = error_context_stack;
+     error_context_stack = &errcallback;
+
      /*
       * Run in short-lived per-tuple context while computing expressions.
       */
*************** ExecTargetList(List *targetlist,
*** 5524,5529 ****
--- 5578,5584 ----
               */
              *isDone = ExprEndResult;
              MemoryContextSwitchTo(oldContext);
+             error_context_stack = errcallback.previous;
              return false;
          }
          else
*************** ExecTargetList(List *targetlist,
*** 5587,5592 ****
--- 5642,5648 ----
                  }

                  MemoryContextSwitchTo(oldContext);
+                 error_context_stack = errcallback.previous;
                  return false;
              }
          }
*************** ExecTargetList(List *targetlist,
*** 5595,5600 ****
--- 5651,5658 ----
      /* Report success */
      MemoryContextSwitchTo(oldContext);

+     error_context_stack = errcallback.previous;
+
      return true;
  }

*************** ExecProject(ProjectionInfo *projInfo, Ex
*** 5616,5621 ****
--- 5674,5680 ----
  {
      TupleTableSlot *slot;
      ExprContext *econtext;
+     ErrorContextCallback errcallback;
      int            numSimpleVars;

      /*
*************** ExecProject(ProjectionInfo *projInfo, Ex
*** 5629,5634 ****
--- 5688,5700 ----
      slot = projInfo->pi_slot;
      econtext = projInfo->pi_exprContext;

+     /* Setup error traceback support for ereport() */
+     econtext->cur_expr = NULL;
+     errcallback.callback = ExprErrorCallback;
+     errcallback.arg = (void *) econtext;
+     errcallback.previous = error_context_stack;
+     error_context_stack = &errcallback;
+
      /* Assume single result row until proven otherwise */
      if (isDone)
          *isDone = ExprSingleResult;
*************** ExecProject(ProjectionInfo *projInfo, Ex
*** 5714,5722 ****
--- 5780,5793 ----
                              slot->tts_isnull,
                              projInfo->pi_itemIsDone,
                              isDone))
+         {
+             error_context_stack = errcallback.previous;
              return slot;        /* no more result rows, return empty slot */
+         }
      }

+     error_context_stack = errcallback.previous;
+
      /*
       * Successfully formed a result row.  Mark the result slot as containing a
       * valid virtual tuple.
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 1688310..583cf43 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
***************
*** 29,34 ****
--- 29,35 ----
  #include "executor/executor.h"
  #include "executor/functions.h"
  #include "funcapi.h"
+ #include "mb/pg_wchar.h"
  #include "miscadmin.h"
  #include "nodes/makefuncs.h"
  #include "nodes/nodeFuncs.h"
*************** sql_inline_error_callback(void *arg)
*** 4698,4703 ****
--- 4699,4727 ----
      errcontext("SQL function \"%s\" during inlining", callback_arg->proname);
  }

+ static void
+ ExprErrorCallback(void *arg)
+ {
+     ExprContext *econtext = (ExprContext *) arg;
+
+     if (econtext->cur_expr)
+     {
+         int            location = exprLocation((Node *) econtext->cur_expr->expr);
+         int            pos;
+
+         /* No-op if location was not provided */
+         if (location < 0)
+             return;
+         /* Can't do anything if source text is not available */
+         if (debug_query_string == NULL)
+             return;
+         /* Convert offset to character number */
+         pos = pg_mbstrlen_with_len(debug_query_string, location) + 1;
+         /* And pass it to the ereport mechanism */
+         errposition(pos);
+     }
+ }
+
  /*
   * evaluate_expr: pre-evaluate a constant expression
   *
*************** evaluate_expr(Expr *expr, Oid result_typ
*** 4710,4715 ****
--- 4734,4741 ----
  {
      EState       *estate;
      ExprState  *exprstate;
+     ExprContext *econtext;
+     ErrorContextCallback errcallback;
      MemoryContext oldcontext;
      Datum        const_val;
      bool        const_is_null;
*************** evaluate_expr(Expr *expr, Oid result_typ
*** 4733,4738 ****
--- 4759,4772 ----
       */
      exprstate = ExecInitExpr(expr, NULL);

+     /* Setup error traceback support for ereport() */
+     econtext = GetPerTupleExprContext(estate);
+     econtext->cur_expr = NULL;
+     errcallback.callback = ExprErrorCallback;
+     errcallback.arg = (void *) econtext;
+     errcallback.previous = error_context_stack;
+     error_context_stack = &errcallback;
+
      /*
       * And evaluate it.
       *
*************** evaluate_expr(Expr *expr, Oid result_typ
*** 4742,4750 ****
       * not depend on context, by definition, n'est ce pas?
       */
      const_val = ExecEvalExprSwitchContext(exprstate,
!                                           GetPerTupleExprContext(estate),
                                            &const_is_null, NULL);

      /* Get info needed about result datatype */
      get_typlenbyval(result_type, &resultTypLen, &resultTypByVal);

--- 4776,4786 ----
       * not depend on context, by definition, n'est ce pas?
       */
      const_val = ExecEvalExprSwitchContext(exprstate,
!                                           econtext,
                                            &const_is_null, NULL);

+     error_context_stack = errcallback.previous;
+
      /* Get info needed about result datatype */
      get_typlenbyval(result_type, &resultTypLen, &resultTypByVal);

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f6f73f3..a390206 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct ExprContext
*** 147,152 ****
--- 147,155 ----
      Datum        domainValue_datum;
      bool        domainValue_isNull;

+     /* Currently executing expression node state, or NULL if indeterminate */
+     struct ExprState *cur_expr;
+
      /* Link to containing EState (NULL if a standalone ExprContext) */
      struct EState *ecxt_estate;