From: Chris Bandy <bandy.chris@gmail.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: 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: Tue, 3 Mar 2020 23:18:51 -0600
Message-ID: <b129e6a8-ccc5-c8eb-01e0-0847ef0390d0@gmail.com> (raw)
In-Reply-To: <20200303160846.GA2728@alvherre.pgsql>
References: <20200303160846.GA2728@alvherre.pgsql>
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
Attachments:
[text/x-patch] v3-0001-Add-schema-and-table-names-to-partition-errors.patch (3.1K, ../b129e6a8-ccc5-c8eb-01e0-0847ef0390d0@gmail.com/2-v3-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 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.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] v3-0002-Tests-for-partition-error-fields.patch (5.4K, ../b129e6a8-ccc5-c8eb-01e0-0847ef0390d0@gmail.com/3-v3-0002-Tests-for-partition-error-fields.patch)
download | inline diff:
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.outnew file mode 100644index 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_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..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
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, alvherre@2ndquadrant.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: <b129e6a8-ccc5-c8eb-01e0-0847ef0390d0@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