Thank you both for look at this!
On 3/1/20 5:14 AM, Amit Kapila wrote:
On Sun, Mar 1, 2020 at 10:10 AM Amit Langote <amitlangot...@gmail.com> wrote:
Hi Chris,
On Sun, Mar 1, 2020 at 4:34 AM Chris Bandy <bandy.ch...@gmail.com> wrote:
Hello,
I'm writing telemetry data into a table partitioned by time. When there
is no partition for a particular date, my app notices the constraint
violation, creates the partition, and retries the insert.
I'm used to handling constraint violations by observing the constraint
name in the error fields. However, this error had none. I set out to add
the name to the error field, but after a bit of reading my impression is
that partition constraints are more like a property of a table.
This makes sense to me. Btree code which implements unique
constraints also does this; see _bt_check_unique() function in
src/backend/access/nbtree/nbtinsert.c:
ereport(ERROR,
(errcode(ERRCODE_UNIQUE_VIOLATION),
errmsg("duplicate key value violates
unique constraint \"%s\"",
RelationGetRelationName(rel)),
key_desc ? errdetail("Key %s already exists.",
key_desc) : 0,
errtableconstraint(heapRel,
RelationGetRelationName(rel))));
I've attached a patch that adds the schema and table name fields to
errors for my use case:
Instead of using errtable(), use errtableconstraint() like the btree
code does, if only just for consistency.
There are two relations in the example you give: the index, rel, and the
table, heapRel. It makes sense to me that two error fields be filled in
with those two names.
With partitions, there is no second name because there is no index nor
constraint object. My (very limited) understanding is that partition
"constraints" are entirely contained within pg_class.relpartbound of the
partition.
Are you suggesting that the table name go into the constraint name field
of the error?
+1. We use errtableconstraint at other places where we use error code
ERRCODE_CHECK_VIOLATION.
Yes, I see this function used when it is a CHECK constraint that is
being violated. In every case the constraint name is passed as the
second argument.
There are couple more instances in src/backend/command/tablecmds.c
where partition constraint is checked:
Maybe, better fix these too for completeness.
Done. As there is no named constraint here, I used errtable again.
Right, if we want to make a change for this, then I think we can once
check all the places where we use error code ERRCODE_CHECK_VIOLATION.
I've looked at every instance of this. It is used for 1) check
constraints, 2) domain constraints, and 3) partition constraints.
1. errtableconstraint is used with the name of the constraint.
2. errdomainconstraint is used with the name of the constraint except in
one instance which deliberately uses errtablecol.
3. With the attached patch, errtable is used except for one instance in
src/backend/partitioning/partbounds.c described below.
In check_default_partition_contents of
src/backend/partitioning/partbounds.c, the default partition is checked
for any rows that should belong in the partition being added _unless_
the leaf being checked is a foreign table. There are two tables
mentioned in this warning, and I couldn't decide which, if any, deserves
to be in the error fields:
/*
* Only RELKIND_RELATION relations (i.e. leaf
partitions) need to be
* scanned.
*/
if (part_rel->rd_rel->relkind != RELKIND_RELATION)
{
if (part_rel->rd_rel->relkind ==
RELKIND_FOREIGN_TABLE)
ereport(WARNING,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("skipped
scanning foreign table \"%s\" which is a partition of default partition
\"%s\"",
RelationGetRelationName(part_rel),
RelationGetRelationName(default_rel))));
if (RelationGetRelid(default_rel) !=
RelationGetRelid(part_rel))
table_close(part_rel, NoLock);
continue;
}
Another thing we might need to see is which of these can be
back-patched. We should also try to write the tests for cases we are
changing even if we don't want to commit those.
I don't have any opinion on back-patching. Existing tests pass. I wasn't
able to find another test that checks the constraint field of errors.
There's a little bit in the tests for psql, but that is about the the
\errverbose functionality rather than specific errors and their fields.
Here's what I tested:
# CREATE TABLE t1 (i int PRIMARY KEY); INSERT INTO t1 VALUES (1), (1);
# \errverbose
...
CONSTRAINT NAME: t1_pkey
# CREATE TABLE pt1 (x int, y int, PRIMARY KEY (x,y)) PARTITIONED BY
RANGE (y);
# INSERT INTO pt1 VALUES (10,10);
# \errverbose
...
SCHEMA NAME: public
TABLE NAME: pt1
# CREATE TABLE pt1_p1 PARTITION OF pt1 FOR VALUES FROM (1) TO (5);
# INSERT INTO pt1 VALUES (10,10);
# \errverbose
...
SCHEMA NAME: public
TABLE NAME: pt1
# INSERT INTO pt1_p1 VALUES (10,10);
# \errverbose
...
SCHEMA NAME: public
TABLE NAME: pt1_p1
# CREATE TABLE pt1_dp PARTITION OF pt1 DEFAULT;
# INSERT INTO pt1 VALUES (10,10);
# CREATE TABLE pt1_p2 PARTITION OF pt1 FOR VALUES FROM (6) TO (20);
# \errverbose
...
SCHEMA NAME: public
TABLE NAME: pt1_dp
Thanks,
Chris
>From c6b39accf9f51f9c08a2fc62e848144776e23ffb Mon Sep 17 00:00:00 2001
From: Chris Bandy <bandy.ch...@gmail.com>
Date: Sat, 29 Feb 2020 12:47:56 -0600
Subject: [PATCH] Add schema and table names to partition errors
---
src/backend/commands/tablecmds.c | 6 ++++--
src/backend/executor/execMain.c | 3 ++-
src/backend/executor/execPartition.c | 3 ++-
src/backend/partitioning/partbounds.c | 3 ++-
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git src/backend/commands/tablecmds.c src/backend/commands/tablecmds.c
index 02a7c04fdb..6a32812a1f 100644
--- src/backend/commands/tablecmds.c
+++ src/backend/commands/tablecmds.c
@@ -5342,12 +5342,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("updated partition constraint for default partition \"%s\" would be violated by some row",
- RelationGetRelationName(oldrel))));
+ RelationGetRelationName(oldrel)),
+ errtable(oldrel)));
else
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("partition constraint of relation \"%s\" is violated by some row",
- RelationGetRelationName(oldrel))));
+ RelationGetRelationName(oldrel)),
+ errtable(oldrel)));
}
/* Write the tuple out to the new relation */
diff --git src/backend/executor/execMain.c src/backend/executor/execMain.c
index ee5c3a60ff..7cb486b211 100644
--- src/backend/executor/execMain.c
+++ src/backend/executor/execMain.c
@@ -1878,7 +1878,8 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("new row for relation \"%s\" violates partition constraint",
RelationGetRelationName(resultRelInfo->ri_RelationDesc)),
- val_desc ? errdetail("Failing row contains %s.", val_desc) : 0));
+ val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
+ errtable(resultRelInfo->ri_RelationDesc)));
}
/*
diff --git src/backend/executor/execPartition.c src/backend/executor/execPartition.c
index c13b1d3501..a5542b92c7 100644
--- src/backend/executor/execPartition.c
+++ src/backend/executor/execPartition.c
@@ -345,7 +345,8 @@ ExecFindPartition(ModifyTableState *mtstate,
RelationGetRelationName(rel)),
val_desc ?
errdetail("Partition key of the failing row contains %s.",
- val_desc) : 0));
+ val_desc) : 0,
+ errtable(rel)));
}
if (partdesc->is_leaf[partidx])
diff --git src/backend/partitioning/partbounds.c src/backend/partitioning/partbounds.c
index 35953f23fa..4c47f54a57 100644
--- src/backend/partitioning/partbounds.c
+++ src/backend/partitioning/partbounds.c
@@ -1366,7 +1366,8 @@ check_default_partition_contents(Relation parent, Relation default_rel,
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("updated partition constraint for default partition \"%s\" would be violated by some row",
- RelationGetRelationName(default_rel))));
+ RelationGetRelationName(default_rel)),
+ errtable(default_rel)));
ResetExprContext(econtext);
CHECK_FOR_INTERRUPTS();
--
2.11.0
