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

Thank you both for look at this!

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:
>>
>> Hi Chris,
>>
>> On Sun, Mar 1, 2020 at 4:34 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>>> Hello,
>>>
>>> I'm writing telemetry data into a table partitioned by time. When there
>>> is no partition for a particular date, my app notices the constraint
>>> violation, creates the partition, and retries the insert.
>>>
>>> I'm used to handling constraint violations by observing the constraint
>>> name in the error fields. However, this error had none. I set out to add
>>> the name to the error field, but after a bit of reading my impression is
>>> that partition constraints are more like a property of a table.
>>
>> 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.

With partitions, there is no second name because there is no index nor 
constraint object. My (very limited) understanding is that partition 
"constraints" are entirely contained within pg_class.relpartbound of the 
partition.

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

> +1.  We use errtableconstraint at other places where we use error code
> ERRCODE_CHECK_VIOLATION.

Yes, I see this function used when it is a CHECK constraint that is 
being violated. In every case the constraint name is passed as the 
second argument.

>> 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))));

                         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.

Here's what I tested:

# CREATE TABLE t1 (i int PRIMARY KEY); INSERT INTO t1 VALUES (1), (1);
# \errverbose
...
CONSTRAINT NAME:  t1_pkey

# CREATE TABLE pt1 (x int, y int, PRIMARY KEY (x,y)) PARTITIONED BY 
RANGE (y);
# INSERT INTO pt1 VALUES (10,10);
# \errverbose
...
SCHEMA NAME:  public
TABLE NAME:  pt1

# CREATE TABLE pt1_p1 PARTITION OF pt1 FOR VALUES FROM (1) TO (5);
# INSERT INTO pt1 VALUES (10,10);
# \errverbose
...
SCHEMA NAME:  public
TABLE NAME:  pt1

# INSERT INTO pt1_p1 VALUES (10,10);
# \errverbose
...
SCHEMA NAME:  public
TABLE NAME:  pt1_p1

# CREATE TABLE pt1_dp PARTITION OF pt1 DEFAULT;
# INSERT INTO pt1 VALUES (10,10);
# CREATE TABLE pt1_p2 PARTITION OF pt1 FOR VALUES FROM (6) TO (20);
# \errverbose
...
SCHEMA NAME:  public
TABLE NAME:  pt1_dp


Thanks,
Chris