From: Chris Bandy <bandy.chris@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add schema and table names to partition error
Date: Sun, 1 Mar 2020 17:51:24 -0600
Message-ID: <bdeb5e60-6a86-6eeb-d073-f489bf3e8f55@gmail.com> (raw)
In-Reply-To: <CAA4eK1JZUxu9x0eQX7BCBGZ+9CvLr91V9OphHsSkE09jc2DQ8Q@mail.gmail.com>
References: <0aa113a3-3c7f-db48-bcd8-f9290b2269ae@gmail.com>
<CA+HiwqGRCm0HsXk_HOyWiPYbmy74QOdxQoHHohKUW1dBiFTtuw@mail.gmail.com>
<CAA4eK1JZUxu9x0eQX7BCBGZ+9CvLr91V9OphHsSkE09jc2DQ8Q@mail.gmail.com>
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 <amitlangote09@gmail.com> wrote:
>>
>> Hi Chris,
>>
>> On Sun, Mar 1, 2020 at 4:34 AM Chris Bandy <bandy.chris@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
Attachments:
[text/x-patch] v2-0001-Add-schema-and-table-names-to-partition-errors.patch (3.1K, ../bdeb5e60-6a86-6eeb-d073-f489bf3e8f55@gmail.com/2-v2-0001-Add-schema-and-table-names-to-partition-errors.patch)
download | inline diff:
From c6b39accf9f51f9c08a2fc62e848144776e23ffb Mon Sep 17 00:00:00 2001
From: Chris Bandy <bandy.chris@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.cindex 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.cindex 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.cindex 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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bandy.chris@gmail.com, amit.kapila16@gmail.com, amitlangote09@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Add schema and table names to partition error
In-Reply-To: <bdeb5e60-6a86-6eeb-d073-f489bf3e8f55@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox