Thread: BUG #8408: Not exactly correct error messages for failed check in presence of dropped columns.
BUG #8408: Not exactly correct error messages for failed check in presence of dropped columns.
From
maxim.boguk@gmail.com
Date:
The following bug has been logged on the website: Bug reference: 8408 Logged by: Maxim Boguk Email address: maxim.boguk@gmail.com PostgreSQL version: 9.2.4 Operating system: Any Description: There are simple test case: create table test_columns (id serial primary key, value int check(value<10)); NOTICE: CREATE TABLE will create implicit sequence "test_columns_id_seq" for serial column "test_columns.id" NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "test_columns_pkey" for table "test_columns" CREATE TABLE insert into test_columns(value) values (100); ERROR: new row for relation "test_columns" violates check constraint "test_columns_value_check" DETAIL: Failing row contains (1, 100). --it's ok... --now let drop value column and add new one alter table test_columns drop column value; ALTER TABLE alter table test_columns add column "value" int check (value < 10); ALTER TABLE insert into test_columns(value) values (100); ERROR: new row for relation "test_columns" violates check constraint "test_columns_value_check" DETAIL: Failing row contains (2, null, 100). -- now (2, null, 100) sound weird... And yes I completely understand where this NULL came from and why this happened. 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). Regards, Maksym
Re: BUG #8408: Not exactly correct error messages for failed check in presence of dropped columns.
From
Michael Paquier
Date:
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.
Breakpoint 1, ExecBuildSlotValueDescription (slot=0x7fc26412b8e0,
maxfieldlen=64) at execMain.c:1636
1636 TupleDesc tupdesc = slot->tts_tupleDescriptor;
(gdb) p *tupdesc->attrs[0]
$1 = {
attrelid = 0,
attname = {
data = "id", '\0' <repeats 61 times>
},
atttypid = 23,
attstattarget = -1,
attlen = 4,
attnum = 1,
attndims = 0,
attcacheoff = 0,
atttypmod = -1,
attbyval = 1 '\001',
attstorage = 112 'p',
attalign = 105 'i',
attnotnull = 0 '\0',
atthasdef = 0 '\0',
attisdropped = 0 '\0',
attislocal = 1 '\001',
attinhcount = 0,
attcollation = 0
}
(gdb) p *tupdesc->attrs[1]
$2 = {
attrelid = 0,
attname = {
data = "........pg.dropped.2........", '\0' <repeats 35 times>
},
atttypid = 23,
attstattarget = -1,
attlen = 4,
attnum = 2,
attndims = 0,
attcacheoff = -1,
atttypmod = -1,
attbyval = 1 '\001',
attstorage = 112 'p',
attalign = 105 'i',
attnotnull = 0 '\0',
atthasdef = 0 '\0',
attisdropped = 0 '\0',
attislocal = 1 '\001',
attinhcount = 0,
attcollation = 0
}
attisdropped is not set to true, only attname is updated to its new value.
When removing an attribute in RemoveAttributeById, it is written in
comment that updating a pg_attribute row triggers a relcache flush for
target relation, but obviously it is not happening. I am missing
something perhaps?
--
Michael
Attachment
Re: BUG #8408: Not exactly correct error messages for failed check in presence of dropped columns.
From
Tom Lane
Date:
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);