Re: [PATCH] Add schema and table names to partition error

Hi Chris,

On Mon, Mar 2, 2020 at 8:51 AM Chris Bandy <bandy(dot)chris(at)gmail(dot)com> wrote:
> On 3/1/20 5:14 AM, Amit Kapila wrote:
> > On Sun, Mar 1, 2020 at 10:10 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >> This makes sense to me. Btree code which implements unique
> >> constraints also does this; see _bt_check_unique() function in
> >> src/backend/access/nbtree/nbtinsert.c:
> >>
> >> ereport(ERROR,
> >> (errcode(ERRCODE_UNIQUE_VIOLATION),
> >> errmsg("duplicate key value violates
> >> unique constraint \"%s\"",
> >> RelationGetRelationName(rel)),
> >> key_desc ? errdetail("Key %s already exists.",
> >> key_desc) : 0,
> >> errtableconstraint(heapRel,
> >>
> >> RelationGetRelationName(rel))));
> >>
> >>> I've attached a patch that adds the schema and table name fields to
> >>> errors for my use case:
> >>
> >> Instead of using errtable(), use errtableconstraint() like the btree
> >> code does, if only just for consistency.
>
> There are two relations in the example you give: the index, rel, and the
> table, heapRel. It makes sense to me that two error fields be filled in
> with those two names.

That's a good point. Index constraints are actually named after the
index and vice versa, so it's a totally valid usage of
errtableconstraint().

create table foo (a int unique);
\d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Indexes:
"foo_a_key" UNIQUE CONSTRAINT, btree (a)

select conname from pg_constraint where conrelid = 'foo'::regclass;
conname
-----------
foo_a_key
(1 row)

create table bar (a int, constraint a_uniq unique (a));
\d bar
Table "public.bar"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Indexes:
"a_uniq" UNIQUE CONSTRAINT, btree (a)

select conname from pg_constraint where conrelid = 'bar'::regclass;
conname
---------
a_uniq
(1 row)

> With partitions, there is no second name because there is no index nor
> constraint object.

It's right to say that partition's case cannot really be equated with
unique indexes.

> My (very limited) understanding is that partition
> "constraints" are entirely contained within pg_class.relpartbound of the
> partition.

That is correct.

> Are you suggesting that the table name go into the constraint name field
> of the error?

Yes, that's what I was thinking, at least for "partition constraint
violated" errors, but given the above that would be a misleading use
of ErrorData.constraint_name.

Maybe it's not too late to invent a new error code like
ERRCODE_PARTITION_VIOLATION or such, then maybe we can use a
hard-coded name, say, just the string "partition constraint".

> >> There are couple more instances in src/backend/command/tablecmds.c
> >> where partition constraint is checked:
> >>
> >> Maybe, better fix these too for completeness.
>
> Done. As there is no named constraint here, I used errtable again.
>
> > Right, if we want to make a change for this, then I think we can once
> > check all the places where we use error code ERRCODE_CHECK_VIOLATION.
>
> I've looked at every instance of this. It is used for 1) check
> constraints, 2) domain constraints, and 3) partition constraints.
>
> 1. errtableconstraint is used with the name of the constraint.
> 2. errdomainconstraint is used with the name of the constraint except in
> one instance which deliberately uses errtablecol.
> 3. With the attached patch, errtable is used except for one instance in
> src/backend/partitioning/partbounds.c described below.
>
> In check_default_partition_contents of
> src/backend/partitioning/partbounds.c, the default partition is checked
> for any rows that should belong in the partition being added _unless_
> the leaf being checked is a foreign table. There are two tables
> mentioned in this warning, and I couldn't decide which, if any, deserves
> to be in the error fields:
>
> /*
> * Only RELKIND_RELATION relations (i.e. leaf
> partitions) need to be
> * scanned.
> */
> if (part_rel->rd_rel->relkind != RELKIND_RELATION)
> {
> if (part_rel->rd_rel->relkind ==
> RELKIND_FOREIGN_TABLE)
> ereport(WARNING,
>
> (errcode(ERRCODE_CHECK_VIOLATION),
> errmsg("skipped
> scanning foreign table \"%s\" which is a partition of default partition
> \"%s\"",
>
> RelationGetRelationName(part_rel),
>
> RelationGetRelationName(default_rel))));

It seems strange to see that errcode here or any errcode for that
matter, so we shouldn't really be concerned about this one.

>
> if (RelationGetRelid(default_rel) !=
> RelationGetRelid(part_rel))
> table_close(part_rel, NoLock);
>
> continue;
> }
>
> > 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.

Thanks,
Amit