Re: BUG #8408: Not exactly correct error messages for failed check in presence of dropped columns. - Mailing list pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Aug 30, 2013 at 2:13 PM,  <maxim.boguk@gmail.com> wrote:
>> However, I think error message should filter dropped column and strange null
>> values (because if a table has a lot dropped column this error message
>> quickly become completely unreadable).

> Yes, this is not really user-friendly. This comes from
> ExecBuildSlotValueDescription@execMain.c when generating a string
> representation of a tuple. Attached is a patch to make this function
> aware of dropped columns when generating the string (usable down to
> 9.2). However, after some debugging I am seeing that the dropped
> column is not seen inside TupleDesc (only the column name is correct)
> when calling this function in executor.

I got around to looking at this finally, and found out that the reason
Michael's patch doesn't work is that the slot's tuple descriptor is one
that's been made by ExecAssignResultTypeFromTL from the result targetlist
the planner generates, so it has no dropped-column markings.  (The
targetlist includes a simple null constant at any dropped column's
position; but TargetEntry has no way to mark itself as a dropped column.)
I think it was coded that way to minimize ExecBuildSlotValueDescription's
API complexity, but really what we ought to do is pass the actual target
relation's tuple descriptor, which will have the necessary markings.
The attached revision of Michael's patch fixes it.

            regards, tom lane

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0b47106..6be17a9 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** static void ExecutePlan(EState *estate,
*** 83,88 ****
--- 83,89 ----
  static bool ExecCheckRTEPerms(RangeTblEntry *rte);
  static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
  static char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
+                               TupleDesc tupdesc,
                                int maxfieldlen);
  static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
                    Plan *planTree);
*************** ExecConstraints(ResultRelInfo *resultRel
*** 1586,1610 ****
                  TupleTableSlot *slot, EState *estate)
  {
      Relation    rel = resultRelInfo->ri_RelationDesc;
!     TupleConstr *constr = rel->rd_att->constr;

      Assert(constr);

      if (constr->has_not_null)
      {
!         int            natts = rel->rd_att->natts;
          int            attrChk;

          for (attrChk = 1; attrChk <= natts; attrChk++)
          {
!             if (rel->rd_att->attrs[attrChk - 1]->attnotnull &&
                  slot_attisnull(slot, attrChk))
                  ereport(ERROR,
                          (errcode(ERRCODE_NOT_NULL_VIOLATION),
                           errmsg("null value in column \"%s\" violates not-null constraint",
!                           NameStr(rel->rd_att->attrs[attrChk - 1]->attname)),
                           errdetail("Failing row contains %s.",
!                                    ExecBuildSlotValueDescription(slot, 64)),
                           errtablecol(rel, attrChk)));
          }
      }
--- 1587,1614 ----
                  TupleTableSlot *slot, EState *estate)
  {
      Relation    rel = resultRelInfo->ri_RelationDesc;
!     TupleDesc    tupdesc = RelationGetDescr(rel);
!     TupleConstr *constr = tupdesc->constr;

      Assert(constr);

      if (constr->has_not_null)
      {
!         int            natts = tupdesc->natts;
          int            attrChk;

          for (attrChk = 1; attrChk <= natts; attrChk++)
          {
!             if (tupdesc->attrs[attrChk - 1]->attnotnull &&
                  slot_attisnull(slot, attrChk))
                  ereport(ERROR,
                          (errcode(ERRCODE_NOT_NULL_VIOLATION),
                           errmsg("null value in column \"%s\" violates not-null constraint",
!                               NameStr(tupdesc->attrs[attrChk - 1]->attname)),
                           errdetail("Failing row contains %s.",
!                                    ExecBuildSlotValueDescription(slot,
!                                                                  tupdesc,
!                                                                  64)),
                           errtablecol(rel, attrChk)));
          }
      }
*************** ExecConstraints(ResultRelInfo *resultRel
*** 1619,1625 ****
                       errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
                              RelationGetRelationName(rel), failed),
                       errdetail("Failing row contains %s.",
!                                ExecBuildSlotValueDescription(slot, 64)),
                       errtableconstraint(rel, failed)));
      }
  }
--- 1623,1631 ----
                       errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
                              RelationGetRelationName(rel), failed),
                       errdetail("Failing row contains %s.",
!                                ExecBuildSlotValueDescription(slot,
!                                                              tupdesc,
!                                                              64)),
                       errtableconstraint(rel, failed)));
      }
  }
*************** ExecWithCheckOptions(ResultRelInfo *resu
*** 1663,1669 ****
                       errmsg("new row violates WITH CHECK OPTION for view \"%s\"",
                              wco->viewname),
                       errdetail("Failing row contains %s.",
!                                ExecBuildSlotValueDescription(slot, 64))));
      }
  }

--- 1669,1677 ----
                       errmsg("new row violates WITH CHECK OPTION for view \"%s\"",
                              wco->viewname),
                       errdetail("Failing row contains %s.",
!                                ExecBuildSlotValueDescription(slot,
!                             RelationGetDescr(resultRelInfo->ri_RelationDesc),
!                                                              64))));
      }
  }

*************** ExecWithCheckOptions(ResultRelInfo *resu
*** 1671,1685 ****
   * ExecBuildSlotValueDescription -- construct a string representing a tuple
   *
   * This is intentionally very similar to BuildIndexValueDescription, but
!  * unlike that function, we truncate long field values.  That seems necessary
!  * here since heap field values could be very long, whereas index entries
!  * typically aren't so wide.
   */
  static char *
! ExecBuildSlotValueDescription(TupleTableSlot *slot, int maxfieldlen)
  {
      StringInfoData buf;
!     TupleDesc    tupdesc = slot->tts_tupleDescriptor;
      int            i;

      /* Make sure the tuple is fully deconstructed */
--- 1679,1700 ----
   * ExecBuildSlotValueDescription -- construct a string representing a tuple
   *
   * This is intentionally very similar to BuildIndexValueDescription, but
!  * unlike that function, we truncate long field values (to at most maxfieldlen
!  * bytes).    That seems necessary here since heap field values could be very
!  * long, whereas index entries typically aren't so wide.
!  *
!  * Also, unlike the case with index entries, we need to be prepared to ignore
!  * dropped columns.  We used to use the slot's tuple descriptor to decode the
!  * data, but the slot's descriptor doesn't identify dropped columns, so we
!  * now need to be passed the relation's descriptor.
   */
  static char *
! ExecBuildSlotValueDescription(TupleTableSlot *slot,
!                               TupleDesc tupdesc,
!                               int maxfieldlen)
  {
      StringInfoData buf;
!     bool        write_comma = false;
      int            i;

      /* Make sure the tuple is fully deconstructed */
*************** ExecBuildSlotValueDescription(TupleTable
*** 1694,1699 ****
--- 1709,1718 ----
          char       *val;
          int            vallen;

+         /* ignore dropped columns */
+         if (tupdesc->attrs[i]->attisdropped)
+             continue;
+
          if (slot->tts_isnull[i])
              val = "null";
          else
*************** ExecBuildSlotValueDescription(TupleTable
*** 1706,1713 ****
              val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
          }

!         if (i > 0)
              appendStringInfoString(&buf, ", ");

          /* truncate if needed */
          vallen = strlen(val);
--- 1725,1734 ----
              val = OidOutputFunctionCall(foutoid, slot->tts_values[i]);
          }

!         if (write_comma)
              appendStringInfoString(&buf, ", ");
+         else
+             write_comma = true;

          /* truncate if needed */
          vallen = strlen(val);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 7cc0084..7366392 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** select * from atacc1;
*** 1197,1202 ****
--- 1197,1216 ----
  (1 row)

  drop table atacc1;
+ -- test constraint error reporting in presence of dropped columns
+ create table atacc1 (id serial primary key, value int check (value < 10));
+ insert into atacc1(value) values (100);
+ ERROR:  new row for relation "atacc1" violates check constraint "atacc1_value_check"
+ DETAIL:  Failing row contains (1, 100).
+ alter table atacc1 drop column value;
+ alter table atacc1 add column value int check (value < 10);
+ insert into atacc1(value) values (100);
+ ERROR:  new row for relation "atacc1" violates check constraint "atacc1_value_check"
+ DETAIL:  Failing row contains (2, 100).
+ insert into atacc1(id, value) values (null, 0);
+ ERROR:  null value in column "id" violates not-null constraint
+ DETAIL:  Failing row contains (null, 0).
+ drop table atacc1;
  -- test inheritance
  create table parent (a int, b int, c int);
  insert into parent values (1, 2, 3);
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index a546ba7..0d3b79b 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
*************** select * from atacc1;
*** 874,879 ****
--- 874,888 ----

  drop table atacc1;

+ -- test constraint error reporting in presence of dropped columns
+ create table atacc1 (id serial primary key, value int check (value < 10));
+ insert into atacc1(value) values (100);
+ alter table atacc1 drop column value;
+ alter table atacc1 add column value int check (value < 10);
+ insert into atacc1(value) values (100);
+ insert into atacc1(id, value) values (null, 0);
+ drop table atacc1;
+
  -- test inheritance
  create table parent (a int, b int, c int);
  insert into parent values (1, 2, 3);