From: Chris Bandy <bandy.chris@gmail.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add schema and table names to partition error
Date: Mon, 2 Mar 2020 22:35:19 -0600
Message-ID: <0acc36c9-1aef-1057-0c29-de473c850605@gmail.com> (raw)
In-Reply-To: <CA+HiwqE_1aDezgt4pBcUSU0UeUkowUUw-rE7faBkS07A-QtWuA@mail.gmail.com>
References: <0aa113a3-3c7f-db48-bcd8-f9290b2269ae@gmail.com>
<CA+HiwqGRCm0HsXk_HOyWiPYbmy74QOdxQoHHohKUW1dBiFTtuw@mail.gmail.com>
<CAA4eK1JZUxu9x0eQX7BCBGZ+9CvLr91V9OphHsSkE09jc2DQ8Q@mail.gmail.com>
<bdeb5e60-6a86-6eeb-d073-f489bf3e8f55@gmail.com>
<CA+HiwqE_1aDezgt4pBcUSU0UeUkowUUw-rE7faBkS07A-QtWuA@mail.gmail.com>
On 3/1/20 10:09 PM, Amit Langote wrote:
> Hi Chris,
>
> On Mon, Mar 2, 2020 at 8:51 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>> 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:
>>>>
>>>> There are couple more instances in src/backend/command/tablecmds.c
>>>> where partition constraint is checked:
>>>>
>>>> Maybe, better fix these too for completeness.
>>>
>>> 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.
>
> Actually, it's not a bad idea to use \errverbose to test this.
>
I've added a second patch with tests that cover three of the five errors
touched by the first patch. Rather than \errverbose, I simply \set
VERBOSITY verbose. I could not find a way to exclude the location field
from the output, so those lines will be likely be out of date soon--if
not already.
I couldn't find a way to exercise the errors in tablecmds.c. Does anyone
know how to instigate a table rewrite that would violate partition
constraints? I tried:
ALTER TABLE pterr1 ALTER y TYPE bigint USING (y - 5);
ERROR: 42P16: cannot alter column "y" because it is part of the
partition key of relation "pterr1"
LOCATION: ATPrepAlterColumnType, tablecmds.c:10812
Thanks,
Chris
Attachments:
[text/x-patch] v2-0001-Add-schema-and-table-names-to-partition-errors.patch (3.1K, ../0acc36c9-1aef-1057-0c29-de473c850605@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
[text/x-patch] v2-0002-Tests-for-partition-error-fields.patch (3.9K, ../0acc36c9-1aef-1057-0c29-de473c850605@gmail.com/3-v2-0002-Tests-for-partition-error-fields.patch)
download | inline diff:
From 2d2ab39bcbc7ea13cd04986b0c8aad6f14449ebd Mon Sep 17 00:00:00 2001
From: Chris Bandy <bandy.chris@gmail.com>
Date: Mon, 2 Mar 2020 22:13:28 -0600
Subject: [PATCH] Tests for partition error fields
---
src/test/regress/expected/partition_errors.out | 37 ++++++++++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/sql/partition_errors.sql | 23 ++++++++++++++++
3 files changed, 61 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/partition_errors.out
create mode 100644 src/test/regress/sql/partition_errors.sql
diff --git src/test/regress/expected/partition_errors.out src/test/regress/expected/partition_errors.outnew file mode 100644index 0000000000..c5a3b8c815--- /dev/null+++ src/test/regress/expected/partition_errors.out@@ -0,0 +1,37 @@+--+-- Tests for partition error fields+--+\set VERBOSITY verbose+-- no partitions+CREATE TABLE pterr1 (x int, y int, PRIMARY KEY (x, y)) PARTITION BY RANGE (y);+INSERT INTO pterr1 VALUES (10, 10);+ERROR: 23514: no partition of relation "pterr1" found for row+DETAIL: Partition key of the failing row contains (y) = (10).+SCHEMA NAME: public+TABLE NAME: pterr1+LOCATION: ExecFindPartition, execPartition.c:349+-- outside the only partition+CREATE TABLE pterr1_p1 PARTITION OF pterr1 FOR VALUES FROM (1) TO (5);+INSERT INTO pterr1 VALUES (10, 10);+ERROR: 23514: no partition of relation "pterr1" found for row+DETAIL: Partition key of the failing row contains (y) = (10).+SCHEMA NAME: public+TABLE NAME: pterr1+LOCATION: ExecFindPartition, execPartition.c:349+INSERT INTO pterr1_p1 VALUES (10, 10);+ERROR: 23514: new row for relation "pterr1_p1" violates partition constraint+DETAIL: Failing row contains (10, 10).+SCHEMA NAME: public+TABLE NAME: pterr1_p1+LOCATION: ExecPartitionCheckEmitError, execMain.c:1882+-- conflict with default+CREATE TABLE pterr1_default PARTITION OF pterr1 DEFAULT;+INSERT INTO pterr1 VALUES (10, 10);+CREATE TABLE pterr1_p2 PARTITION OF pterr1 FOR VALUES FROM (6) TO (20);+ERROR: 23514: updated partition constraint for default partition "pterr1_default" would be violated by some row+SCHEMA NAME: public+TABLE NAME: pterr1_default+LOCATION: check_default_partition_contents, partbounds.c:1370+-- cleanup+\set VERBOSITY default+DROP TABLE pterr1, pterr1_default, pterr1_p1;diff --git src/test/regress/parallel_schedule src/test/regress/parallel_scheduleindex d2b17dd3ea..e1708c87ec 100644--- src/test/regress/parallel_schedule+++ src/test/regress/parallel_schedule@@ -112,7 +112,7 @@ test: plancache limit plpgsql copy2 temp domain rangefuncs prepare conversion tr
# ----------
# Another group of parallel tests
# ----------
-test: partition_join partition_prune reloptions hash_part indexing partition_aggregate partition_info tuplesort explain+test: partition_errors partition_join partition_prune reloptions hash_part indexing partition_aggregate partition_info tuplesort explain
# event triggers cannot run concurrently with any test that runs DDL
test: event_trigger
diff --git src/test/regress/sql/partition_errors.sql src/test/regress/sql/partition_errors.sqlnew file mode 100644index 0000000000..7b9197d507--- /dev/null+++ src/test/regress/sql/partition_errors.sql@@ -0,0 +1,23 @@+--+-- Tests for partition error fields+--++\set VERBOSITY verbose++-- no partitions+CREATE TABLE pterr1 (x int, y int, PRIMARY KEY (x, y)) PARTITION BY RANGE (y);+INSERT INTO pterr1 VALUES (10, 10);++-- outside the only partition+CREATE TABLE pterr1_p1 PARTITION OF pterr1 FOR VALUES FROM (1) TO (5);+INSERT INTO pterr1 VALUES (10, 10);+INSERT INTO pterr1_p1 VALUES (10, 10);++-- conflict with default+CREATE TABLE pterr1_default PARTITION OF pterr1 DEFAULT;+INSERT INTO pterr1 VALUES (10, 10);+CREATE TABLE pterr1_p2 PARTITION OF pterr1 FOR VALUES FROM (6) TO (20);++-- cleanup+\set VERBOSITY default+DROP TABLE pterr1, pterr1_default, pterr1_p1;--
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, amitlangote09@gmail.com, amit.kapila16@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Add schema and table names to partition error
In-Reply-To: <0acc36c9-1aef-1057-0c29-de473c850605@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