On 3/3/20 11:18 PM, Chris Bandy wrote:
> On 3/3/20 10:08 AM, Alvaro Herrera wrote:
>> 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
> 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
By the power of grep I found another partition error that needed a
field. I'm pretty happy with the way the test turned out, so I've
squashed everything into a single patch.
I've also convinced myself that the number of integrity errors in the
entire codebase is manageable to test. If others think it is worthwhile,
I can spend some time over the next week to expand this test approach to
cover _all_ SQLSTATE class 23 errors.
If so,
* Should it be one regression test (file) that discusses the
significance of class 23, or
* Should it be a few test cases added to the existing test files related
to each feature?
The former allows the helper function to be defined once, while the
latter would repeat it over many files.
Thanks,
Chris
>From 5fbe19233ed734b52cec77d76e81282c7bf2360a Mon Sep 17 00:00:00 2001
From: Chris Bandy <bandy.ch...@gmail.com>
Date: Sat, 29 Feb 2020 12:47:56 -0600
Subject: [PATCH] Add object names to partition errors
All errors of SQLSTATE class 23 should include the name of an object
associated with the error.
---
src/backend/commands/tablecmds.c | 6 +-
src/backend/executor/execMain.c | 3 +-
src/backend/executor/execPartition.c | 3 +-
src/backend/partitioning/partbounds.c | 3 +-
src/backend/utils/adt/ri_triggers.c | 3 +-
src/test/regress/expected/partition_errors.out | 93 ++++++++++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/sql/partition_errors.sql | 62 +++++++++++++++++
8 files changed, 168 insertions(+), 7 deletions(-)
create mode 100644 src/test/regress/expected/partition_errors.out
create mode 100644 src/test/regress/sql/partition_errors.sql
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 28130fbc2b..4fdffad6f3 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();
diff --git src/backend/utils/adt/ri_triggers.c src/backend/utils/adt/ri_triggers.c
index 4ab7cda110..bb49e80d16 100644
--- src/backend/utils/adt/ri_triggers.c
+++ src/backend/utils/adt/ri_triggers.c
@@ -2452,7 +2452,8 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
NameStr(riinfo->conname)),
errdetail("Key (%s)=(%s) is still referenced from table \"%s\".",
key_names.data, key_values.data,
- RelationGetRelationName(fk_rel))));
+ RelationGetRelationName(fk_rel)),
+ errtableconstraint(fk_rel, NameStr(riinfo->conname))));
else if (onfk)
ereport(ERROR,
(errcode(ERRCODE_FOREIGN_KEY_VIOLATION),
diff --git src/test/regress/expected/partition_errors.out src/test/regress/expected/partition_errors.out
new file mode 100644
index 0000000000..8f3da6c74f
--- /dev/null
+++ src/test/regress/expected/partition_errors.out
@@ -0,0 +1,93 @@
+--
+-- Tests for partition error fields
+--
+-- Some partition errors are emitted with SQLSTATE 23xxx. Such errors
+-- should include the name of a database object as a separate field.
+--
+-- The fields of interest are shown at the same verbosity level as
+-- volatile details such as source-code line numbers. To produce stable
+-- regression output, the following function returns a portion of the
+-- full error reported.
+\pset expanded on
+\pset tuples_only 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,
+ OUT err_constraint text)
+AS $$
+BEGIN
+ EXECUTE $1;
+EXCEPTION
+ WHEN integrity_constraint_violation 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,
+ err_constraint := CONSTRAINT_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);
+$$);
+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
+err_constraint |
+
+-- 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);
+$$);
+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
+err_constraint |
+
+SELECT * FROM partition_error_record($$
+ INSERT INTO pterr1_p1 VALUES (10, 10);
+$$);
+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
+err_constraint |
+
+-- 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);
+$$);
+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
+err_constraint |
+
+-- foreign key reference
+CREATE TABLE pterr2 (x int, y int, FOREIGN KEY (x, y) REFERENCES pterr1);
+INSERT INTO pterr2 VALUES (10, 10);
+SELECT * FROM partition_error_record($$
+ ALTER TABLE pterr1 DETACH PARTITION pterr1_default;
+$$);
+err_sqlstate | 23503
+err_message | removing partition "pterr1_default" violates foreign key constraint "pterr2_x_y_fkey2"
+err_detail | Key (x, y)=(10, 10) is still referenced from table "pterr2".
+err_schema | public
+err_table | pterr2
+err_constraint | pterr2_x_y_fkey2
+
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..054ec2e059
--- /dev/null
+++ src/test/regress/sql/partition_errors.sql
@@ -0,0 +1,62 @@
+--
+-- Tests for partition error fields
+--
+-- Some partition errors are emitted with SQLSTATE 23xxx. Such errors
+-- should include the name of a database object as a separate field.
+--
+-- The fields of interest are shown at the same verbosity level as
+-- volatile details such as source-code line numbers. To produce stable
+-- regression output, the following function returns a portion of the
+-- full error reported.
+\pset expanded on
+\pset tuples_only 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,
+ OUT err_constraint text)
+AS $$
+BEGIN
+ EXECUTE $1;
+EXCEPTION
+ WHEN integrity_constraint_violation 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,
+ err_constraint := CONSTRAINT_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);
+$$);
+
+-- foreign key reference
+CREATE TABLE pterr2 (x int, y int, FOREIGN KEY (x, y) REFERENCES pterr1);
+INSERT INTO pterr2 VALUES (10, 10);
+SELECT * FROM partition_error_record($$
+ ALTER TABLE pterr1 DETACH PARTITION pterr1_default;
+$$);
--
2.11.0
