Subject: Re: [PATCH] Add schema and table names to partition error
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Amit Langote <amitlangote09@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <20200303160846.GA2728@alvherre.pgsql>
From: Chris Bandy <bandy.chris@gmail.com>
Message-ID: <b129e6a8-ccc5-c8eb-01e0-0847ef0390d0@gmail.com>
Date: Tue, 3 Mar 2020 23:18:51 -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: <20200303160846.GA2728@alvherre.pgsql>
Content-Type: multipart/mixed;
 boundary="------------76B59C74AEED26C8BB8A2A55"
Content-Language: en-US
Precedence: bulk

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

On 3/3/20 10:08 AM, Alvaro Herrera wrote:
>> +\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
> 
> This won't work well, because people would be forced to update the .out
> file whenever the execPartition.c file changed to add or remove lines
> before the one with the error call.

I agree. I expected that and should have made it more clear that I 
didn't intend for those tests to be committed. Others in the thread 
suggested I include some form of test, even if it didn't live past 
review. That being said...

> Maybe if you want to verify the
> schema/table names, use a plpgsql function to extract them, using
> GET STACKED DIAGNOSTICS TABLE_NAME = ...
> in an exception block?

This is a great idea and the result looks much cleaner than I expected. 
I have no reservations about committing the attached tests.

> I'm not sure that this *needs* to be tested, though.  Don't we already
> verify that errtable() works, elsewhere?

I looked for tests that might target errtable() or errtableconstraint() 
but found none. Perhaps someone who knows the tests better could answer 
this question.

> I don't suppose you mean to
> test that every single ereport() call that includes errtable() contains
> a TABLE NAME item.

Correct. I intend only to test the few calls I'm touching in this 
thread. It might be worthwhile for someone to perform a more thorough 
review of existing errors, however. The documentation seems to say that 
every error in SQLSTATE class 23 has one of these fields filled[1]. The 
errors in these patches are in that class but lacked any fields.

[1] https://www.postgresql.org/docs/current/errcodes-appendix.html

Thanks,
Chris

--------------76B59C74AEED26C8BB8A2A55
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0001-Add-schema-and-table-names-to-partition-errors.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-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 1/2] 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


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

From 19a259beeab29ca4a1398641c8e2dbdf5a3d548e 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 2/2] Tests for partition error fields

---
 src/test/regress/expected/partition_errors.out | 70 ++++++++++++++++++++++++++
 src/test/regress/parallel_schedule             |  2 +-
 src/test/regress/sql/partition_errors.sql      | 45 +++++++++++++++++
 3 files changed, 116 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..cbe77fd107
--- /dev/null
+++ src/test/regress/expected/partition_errors.out
@@ -0,0 +1,70 @@
+--
+-- Tests for partition error fields
+--
+\pset expanded on
+CREATE FUNCTION partition_error_record(
+    dml text,
+    OUT err_sqlstate text,
+    OUT err_message text,
+    OUT err_detail text,
+    OUT err_schema text,
+    OUT err_table text)
+AS $$
+BEGIN
+    EXECUTE $1;
+EXCEPTION
+    WHEN OTHERS THEN GET STACKED DIAGNOSTICS
+        err_sqlstate := RETURNED_SQLSTATE,
+        err_message := MESSAGE_TEXT,
+        err_detail := PG_EXCEPTION_DETAIL,
+        err_schema := SCHEMA_NAME,
+        err_table := TABLE_NAME;
+END;
+$$ LANGUAGE plpgsql;
+-- no partitions
+CREATE TABLE pterr1 (x int, y int, PRIMARY KEY (x, y)) PARTITION BY RANGE (y);
+SELECT * FROM partition_error_record($$
+    INSERT INTO pterr1 VALUES (10, 10);
+$$);
+-[ RECORD 1 ]+------------------------------------------------------
+err_sqlstate | 23514
+err_message  | no partition of relation "pterr1" found for row
+err_detail   | Partition key of the failing row contains (y) = (10).
+err_schema   | public
+err_table    | pterr1
+
+-- outside the only partition
+CREATE TABLE pterr1_p1 PARTITION OF pterr1 FOR VALUES FROM (1) TO (5);
+SELECT * FROM partition_error_record($$
+    INSERT INTO pterr1 VALUES (10, 10);
+$$);
+-[ RECORD 1 ]+------------------------------------------------------
+err_sqlstate | 23514
+err_message  | no partition of relation "pterr1" found for row
+err_detail   | Partition key of the failing row contains (y) = (10).
+err_schema   | public
+err_table    | pterr1
+
+SELECT * FROM partition_error_record($$
+    INSERT INTO pterr1_p1 VALUES (10, 10);
+$$);
+-[ RECORD 1 ]+---------------------------------------------------------------
+err_sqlstate | 23514
+err_message  | new row for relation "pterr1_p1" violates partition constraint
+err_detail   | Failing row contains (10, 10).
+err_schema   | public
+err_table    | pterr1_p1
+
+-- conflict with default
+CREATE TABLE pterr1_default PARTITION OF pterr1 DEFAULT;
+INSERT INTO pterr1 VALUES (10, 10);
+SELECT * FROM partition_error_record($$
+    CREATE TABLE pterr1_p2 PARTITION OF pterr1 FOR VALUES FROM (6) TO (20);
+$$);
+-[ RECORD 1 ]+--------------------------------------------------------------------------------------------------
+err_sqlstate | 23514
+err_message  | updated partition constraint for default partition "pterr1_default" would be violated by some row
+err_detail   | 
+err_schema   | public
+err_table    | pterr1_default
+
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..420d75994f
--- /dev/null
+++ src/test/regress/sql/partition_errors.sql
@@ -0,0 +1,45 @@
+--
+-- Tests for partition error fields
+--
+\pset expanded on
+CREATE FUNCTION partition_error_record(
+    dml text,
+    OUT err_sqlstate text,
+    OUT err_message text,
+    OUT err_detail text,
+    OUT err_schema text,
+    OUT err_table text)
+AS $$
+BEGIN
+    EXECUTE $1;
+EXCEPTION
+    WHEN OTHERS THEN GET STACKED DIAGNOSTICS
+        err_sqlstate := RETURNED_SQLSTATE,
+        err_message := MESSAGE_TEXT,
+        err_detail := PG_EXCEPTION_DETAIL,
+        err_schema := SCHEMA_NAME,
+        err_table := TABLE_NAME;
+END;
+$$ LANGUAGE plpgsql;
+
+-- no partitions
+CREATE TABLE pterr1 (x int, y int, PRIMARY KEY (x, y)) PARTITION BY RANGE (y);
+SELECT * FROM partition_error_record($$
+    INSERT INTO pterr1 VALUES (10, 10);
+$$);
+
+-- outside the only partition
+CREATE TABLE pterr1_p1 PARTITION OF pterr1 FOR VALUES FROM (1) TO (5);
+SELECT * FROM partition_error_record($$
+    INSERT INTO pterr1 VALUES (10, 10);
+$$);
+SELECT * FROM partition_error_record($$
+    INSERT INTO pterr1_p1 VALUES (10, 10);
+$$);
+
+-- conflict with default
+CREATE TABLE pterr1_default PARTITION OF pterr1 DEFAULT;
+INSERT INTO pterr1 VALUES (10, 10);
+SELECT * FROM partition_error_record($$
+    CREATE TABLE pterr1_p2 PARTITION OF pterr1 FOR VALUES FROM (6) TO (20);
+$$);
-- 
2.11.0


--------------76B59C74AEED26C8BB8A2A55--