Subject: Re: [PATCH] Add schema and table names to partition error
To: Amit Langote <amitlangote09@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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>
From: Chris Bandy <bandy.chris@gmail.com>
Message-ID: <0acc36c9-1aef-1057-0c29-de473c850605@gmail.com>
Date: Mon, 2 Mar 2020 22:35:19 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: 
 <CA+HiwqE_1aDezgt4pBcUSU0UeUkowUUw-rE7faBkS07A-QtWuA@mail.gmail.com>
Content-Type: multipart/mixed;
 boundary="------------11D58237E49522B326EBA8A0"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------11D58237E49522B326EBA8A0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

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

--------------11D58237E49522B326EBA8A0
Content-Type: text/x-patch; charset=UTF-8;
 name="v2-0001-Add-schema-and-table-names-to-partition-errors.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v2-0001-Add-schema-and-table-names-to-partition-errors.patch"

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.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


--------------11D58237E49522B326EBA8A0
Content-Type: text/x-patch; charset=UTF-8;
 name="v2-0002-Tests-for-partition-error-fields.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v2-0002-Tests-for-partition-error-fields.patch"

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.out
new file mode 100644
index 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_schedule
index 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.sql
new file mode 100644
index 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


--------------11D58237E49522B326EBA8A0--