Horton

Thread

Commits

  1. Add object names to partition integrity violations.

  1. [PATCH] Add schema and table names to partition error

    Chris Bandy <bandy.chris@gmail.com> — 2020-02-29T19:33:48Z

    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.

I've attached a patch that adds the schema and table name fields to 
errors for my use case:

- Insert data into a partitioned table for which there is no partition.
- Insert data directly into an incorrect partition.

Thanks,
Chris

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

    Amit Langote <amitlangote09@gmail.com> — 2020-03-01T04:40:08Z

    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.

> - Insert data into a partitioned table for which there is no partition.
> - Insert data directly into an incorrect partition.

There are couple more instances in src/backend/command/tablecmds.c
where partition constraint is checked:

In ATRewriteTable():

            if (partqualstate && !ExecCheck(partqualstate, econtext))
            {
                if (tab->validate_default)
                    ereport(ERROR,
                            (errcode(ERRCODE_CHECK_VIOLATION),
                             errmsg("updated partition constraint for
default partition \"%s\" would be violated by some row",
                                    RelationGetRelationName(oldrel))));
                else
                    ereport(ERROR,
                            (errcode(ERRCODE_CHECK_VIOLATION),
                             errmsg("partition constraint of relation
\"%s\" is violated by some row",
                                    RelationGetRelationName(oldrel))));
            }

Maybe, better fix these too for completeness.

Thanks,
Amit

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-01T11:14:26Z

    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.
>

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

> > - Insert data into a partitioned table for which there is no partition.
> > - Insert data directly into an incorrect partition.
>
> There are couple more instances in src/backend/command/tablecmds.c
> where partition constraint is checked:
>
> In ATRewriteTable():
>
>             if (partqualstate && !ExecCheck(partqualstate, econtext))
>             {
>                 if (tab->validate_default)
>                     ereport(ERROR,
>                             (errcode(ERRCODE_CHECK_VIOLATION),
>                              errmsg("updated partition constraint for
> default partition \"%s\" would be violated by some row",
>                                     RelationGetRelationName(oldrel))));
>                 else
>                     ereport(ERROR,
>                             (errcode(ERRCODE_CHECK_VIOLATION),
>                              errmsg("partition constraint of relation
> \"%s\" is violated by some row",
>                                     RelationGetRelationName(oldrel))));
>             }
>
> Maybe, better fix these too for completeness.
>

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.
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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-01T23:51:24Z

    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

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

    Amit Langote <amitlangote09@gmail.com> — 2020-03-02T04:09:14Z

    Hi Chris,

On Mon, Mar 2, 2020 at 8:51 AM Chris Bandy <bandy.chris@gmail.com> wrote:
> 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:
> >> 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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-02T11:00:07Z

    On Mon, Mar 2, 2020 at 9:39 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
>
> > 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.
>

I think it is better to use errtable in such cases.

> 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.
>

Right.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-03T04:35:19Z

    On 3/1/20 10:09 PM, Amit Langote wrote:
> Hi Chris,
> 
> On Mon, Mar 2, 2020 at 8:51 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>> 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:
>>>>
>>>> There are couple more instances in src/backend/command/tablecmds.c
>>>> where partition constraint is checked:
>>>>
>>>> Maybe, better fix these too for completeness.
>>>
>>> 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.
> 

I've added a second patch with tests that cover three of the five errors 
touched by the first patch. Rather than \errverbose, I simply \set 
VERBOSITY verbose. I could not find a way to exclude the location field 
from the output, so those lines will be likely be out of date soon--if 
not already.

I couldn't find a way to exercise the errors in tablecmds.c. Does anyone 
know how to instigate a table rewrite that would violate partition 
constraints? I tried:

ALTER TABLE pterr1 ALTER y TYPE bigint USING (y - 5);
ERROR:  42P16: cannot alter column "y" because it is part of the 
partition key of relation "pterr1"
LOCATION:  ATPrepAlterColumnType, tablecmds.c:10812

Thanks,
Chris

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-03T16:08:46Z

    > +\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.  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?

I'm not sure that this *needs* to be tested, though.  Don't we already
verify that errtable() works, elsewhere?  I don't suppose you mean to
test that every single ereport() call that includes errtable() contains
a TABLE NAME item.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-04T05:18:51Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-04T05:22:44Z

    On Wed, Mar 4, 2020 at 10:48 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>
> 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.
>

Right, it is not for committing those tests, but rather once we try to
hit the code we are changing in this patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  11. Re: [PATCH] Add object names to partition errors

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-04T08:54:20Z

    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

  12. [PATCH] Add tests for integrity violation error fields

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-07T05:37:37Z

    On 3/4/20 2:54 AM, Chris Bandy wrote:
> 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.

Done. Please find attached two patches that (1) test all but one reports
of integrity violations and (2) attach object names to the handful that
lacked them.

I decided to include error messages in the tests so that the next person
to change the message would be mindful of the attached fields and vice
versa. I thought these might be impacted by locale, but `make check
LANG=de_DE.utf8` passes for me. Is that command the right way to verify
that?

With these patches, behavior matches the documentation which states:
"[object] names are supplied in separate fields of the error report
message so that applications need not try to extract them from the
possibly-localized human-readable text of the message. As of PostgreSQL
9.3, complete coverage for this feature exists only for errors in
SQLSTATE class 23..."


Thanks,
Chris

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-11T11:29:02Z

    On Tue, Mar 3, 2020 at 10:05 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>
> On 3/1/20 10:09 PM, Amit Langote wrote:
> > Hi Chris,
> >
> > On Mon, Mar 2, 2020 at 8:51 AM Chris Bandy <bandy.chris@gmail.com> wrote:
> >> 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:
> >>>>
> >>>> There are couple more instances in src/backend/command/tablecmds.c
> >>>> where partition constraint is checked:
> >>>>
> >>>> Maybe, better fix these too for completeness.
> >>>
> >>> 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.
> >
>
> I've added a second patch with tests that cover three of the five errors
> touched by the first patch. Rather than \errverbose, I simply \set
> VERBOSITY verbose. I could not find a way to exclude the location field
> from the output, so those lines will be likely be out of date soon--if
> not already.
>
> I couldn't find a way to exercise the errors in tablecmds.c. Does anyone
> know how to instigate a table rewrite that would violate partition
> constraints? I tried:
>

When I tried to apply your patch on HEAD with patch -p1 <
<path_to_patch>, I am getting below errors

(Stripping trailing CRs from patch; use --binary to disable.)
can't find file to patch at input line 17
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
..

I have tried with git am as well, but it failed.  I am not sure what
is the reason.  Can you please once check at your end? Also, see, if
it applies till 9.5 as I think we should backpatch this.

IIUC, this patch is mainly to get the table name, schema name in case
of the error paths, so that your application can handle errors in case
partition constraint violation, right?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-11T15:21:45Z

    Amit,

On 3/11/20 6:29 AM, Amit Kapila wrote:
> On Tue, Mar 3, 2020 at 10:05 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>>
>> On 3/1/20 10:09 PM, Amit Langote wrote:
>>> Hi Chris,
>>>
>>> On Mon, Mar 2, 2020 at 8:51 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>>>> 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:
>>>>>>
>>>>>> There are couple more instances in src/backend/command/tablecmds.c
>>>>>> where partition constraint is checked:
>>>>>>
>>>>>> Maybe, better fix these too for completeness.
>>>>>
>>>>> 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.
>>>
>>
>> I've added a second patch with tests that cover three of the five errors
>> touched by the first patch. Rather than \errverbose, I simply \set
>> VERBOSITY verbose. I could not find a way to exclude the location field
>> from the output, so those lines will be likely be out of date soon--if
>> not already.
>>
>> I couldn't find a way to exercise the errors in tablecmds.c. Does anyone
>> know how to instigate a table rewrite that would violate partition
>> constraints? I tried:
>>
> 
> When I tried to apply your patch on HEAD with patch -p1 <
> <path_to_patch>, I am getting below errors
> 
> (Stripping trailing CRs from patch; use --binary to disable.)
> can't find file to patch at input line 17
> Perhaps you used the wrong -p or --strip option?
> The text leading up to this was:
> ..
> 
> I have tried with git am as well, but it failed.  I am not sure what
> is the reason.  Can you please once check at your end?

Yes, sorry. This set (and v3 and v4) should work with -p0. Any following
patches from me will use the normal -p1.

> Also, see, if
> it applies till 9.5 as I think we should backpatch this.
> 
> IIUC, this patch is mainly to get the table name, schema name in case
> of the error paths, so that your application can handle errors in case
> partition constraint violation, right?

Yes, that is correct. Which also means it doesn't apply to 9.5 (no
partitions!) Later in this thread I created a test that covers all
integrity violation errors.[1] *That* can be backpatched, if you'd like.

For an approach limited to partitions only, I recommend looking at v4
rather than v2 or v3.[2]

[1]: https://postgresql.org/message-id/0731def8-978e-0285-04ee-582762729b38%40gmail.com
[2]: https://postgresql.org/message-id/7985cf2f-5082-22d9-1bb4-6b280150eeae%40gmail.com

Thanks,
Chris

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-12T14:16:20Z

    On Wed, Mar 11, 2020 at 8:51 PM Chris Bandy <bandy.chris@gmail.com> wrote:
>
> On 3/11/20 6:29 AM, Amit Kapila wrote:
> >
> > I have tried with git am as well, but it failed.  I am not sure what
> > is the reason.  Can you please once check at your end?
>
> Yes, sorry. This set (and v3 and v4) should work with -p0. Any following
> patches from me will use the normal -p1.
>

Okay.

> > Also, see, if
> > it applies till 9.5 as I think we should backpatch this.
> >
> > IIUC, this patch is mainly to get the table name, schema name in case
> > of the error paths, so that your application can handle errors in case
> > partition constraint violation, right?
>
> Yes, that is correct. Which also means it doesn't apply to 9.5 (no
> partitions!) Later in this thread I created a test that covers all
> integrity violation errors.[1] *That* can be backpatched, if you'd like.
>
> For an approach limited to partitions only, I recommend looking at v4
> rather than v2 or v3.[2]
>

It is strange that I didn't receive your email which has a v4 version.
I will look into it, but I don't think we need to add the tests for
error conditions. Those are good for testing, but I think if we start
adding tests for all error conditions, then it might increase the
number of tests that are not of very high value.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-18T11:56:19Z

    On Thu, Mar 12, 2020 at 7:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 11, 2020 at 8:51 PM Chris Bandy <bandy.chris@gmail.com> wrote:
> >
> > On 3/11/20 6:29 AM, Amit Kapila wrote:
> > >
> > > I have tried with git am as well, but it failed.  I am not sure what
> > > is the reason.  Can you please once check at your end?
> >
> > Yes, sorry. This set (and v3 and v4) should work with -p0. Any following
> > patches from me will use the normal -p1.
> >
>
> Okay.
>

I again tried the latest patch v5 both with -p1 and -p0, but it gives
an error while applying the patch.  Can you send a patch that we can
apply with patch -p1 or git-am?

[1] - https://www.postgresql.org/message-id/0731def8-978e-0285-04ee-582762729b38%40gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-18T22:25:05Z

    On 3/18/20 6:56 AM, Amit Kapila wrote:
> On Thu, Mar 12, 2020 at 7:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, Mar 11, 2020 at 8:51 PM Chris Bandy <bandy.chris@gmail.com> wrote:
>>>
>>> On 3/11/20 6:29 AM, Amit Kapila wrote:
>>>>
>>>> I have tried with git am as well, but it failed.  I am not sure what
>>>> is the reason.  Can you please once check at your end?
>>>
>>> Yes, sorry. This set (and v3 and v4) should work with -p0. Any following
>>> patches from me will use the normal -p1.
>>>
>>
>> Okay.
>>
> 
> I again tried the latest patch v5 both with -p1 and -p0, but it gives
> an error while applying the patch.  Can you send a patch that we can
> apply with patch -p1 or git-am?
> 
> [1] - https://www.postgresql.org/message-id/0731def8-978e-0285-04ee-582762729b38%40gmail.com
> 

Sorry for these troubles. Attached are patches created using `git
format-patch -n -v6` on master at 487e9861d0.

Thanks,
Chris

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-19T04:46:08Z

    On Thu, Mar 19, 2020 at 3:55 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>
>
> Sorry for these troubles. Attached are patches created using `git
> format-patch -n -v6` on master at 487e9861d0.
>

No problem.  I have extracted your code changes as a separate patch
(see attached) as I am not sure we want to add tests for these cases.
This doesn't apply in back-branches, but I think that is small work
and we can do that if required.  The real question is do we want to
back-patch this?  Basically, this improves the errors in certain cases
by providing additional information that otherwise the user might need
to extract from error messages.  So, there doesn't seem to be pressing
need to back-patch this but OTOH, we have mentioned in docs that we
support to display this information for all SQLSTATE class 23
(integrity constraint violation) errors which is not true as we forgot
to adhere to that in some parts of code.

What do you think?  Anybody else has an opinion on whether to
back-patch this or not?

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

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Amit Langote <amitlangote09@gmail.com> — 2020-03-19T10:04:47Z

    Thank you Chris, Amit.

On Thu, Mar 19, 2020 at 1:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Mar 19, 2020 at 3:55 AM Chris Bandy <bandy.chris@gmail.com> wrote:
> >
> >
> > Sorry for these troubles. Attached are patches created using `git
> > format-patch -n -v6` on master at 487e9861d0.
> >
>
> No problem.  I have extracted your code changes as a separate patch
> (see attached) as I am not sure we want to add tests for these cases.
> This doesn't apply in back-branches, but I think that is small work
> and we can do that if required.  The real question is do we want to
> back-patch this?  Basically, this improves the errors in certain cases
> by providing additional information that otherwise the user might need
> to extract from error messages.  So, there doesn't seem to be pressing
> need to back-patch this but OTOH, we have mentioned in docs that we
> support to display this information for all SQLSTATE class 23
> (integrity constraint violation) errors which is not true as we forgot
> to adhere to that in some parts of code.
>
> What do you think?  Anybody else has an opinion on whether to
> back-patch this or not?

As nobody except Chris complained about this so far, maybe no?

-- 
Thank you,
Amit

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

    Chris Bandy <bandy.chris@gmail.com> — 2020-03-19T14:51:15Z

    On 3/18/20 11:46 PM, Amit Kapila wrote:
> On Thu, Mar 19, 2020 at 3:55 AM Chris Bandy <bandy.chris@gmail.com> wrote:
>>
>>
>> Sorry for these troubles. Attached are patches created using `git
>> format-patch -n -v6` on master at 487e9861d0.
>>
> 
> No problem.  I have extracted your code changes as a separate patch
> (see attached) as I am not sure we want to add tests for these cases.

Patch looks good.

My last pitch to keep the tests: These would be the first and only
automated tests that verify errtable, errtableconstraint, etc.

> This doesn't apply in back-branches, but I think that is small work
> and we can do that if required.

It looks like the only failing hunk on REL_12_STABLE is in tablecmds.c.
The ereport is near line 5090 there. The partition code has changed
quite a bit compared the older branches. ;-)

Thanks,
Chris

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-20T06:50:32Z

    On Thu, Mar 19, 2020 at 8:21 PM Chris Bandy <bandy.chris@gmail.com> wrote:
>
> On 3/18/20 11:46 PM, Amit Kapila wrote:
> > On Thu, Mar 19, 2020 at 3:55 AM Chris Bandy <bandy.chris@gmail.com> wrote:
> >>
> >>
> >> Sorry for these troubles. Attached are patches created using `git
> >> format-patch -n -v6` on master at 487e9861d0.
> >>
> >
> > No problem.  I have extracted your code changes as a separate patch
> > (see attached) as I am not sure we want to add tests for these cases.
>
> Patch looks good.
>
> My last pitch to keep the tests: These would be the first and only
> automated tests that verify errtable, errtableconstraint, etc.
>

I don't object to those tests.  However, I don't feel adding just for
this patch is advisable.  I suggest you start a new thread for these
tests and let us see what others think about them.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-20T06:52:01Z

    On Thu, Mar 19, 2020 at 3:34 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Thank you Chris, Amit.
>
> On Thu, Mar 19, 2020 at 1:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Thu, Mar 19, 2020 at 3:55 AM Chris Bandy <bandy.chris@gmail.com> wrote:
> > >
> > >
> > > Sorry for these troubles. Attached are patches created using `git
> > > format-patch -n -v6` on master at 487e9861d0.
> > >
> >
> > No problem.  I have extracted your code changes as a separate patch
> > (see attached) as I am not sure we want to add tests for these cases.
> > This doesn't apply in back-branches, but I think that is small work
> > and we can do that if required.  The real question is do we want to
> > back-patch this?  Basically, this improves the errors in certain cases
> > by providing additional information that otherwise the user might need
> > to extract from error messages.  So, there doesn't seem to be pressing
> > need to back-patch this but OTOH, we have mentioned in docs that we
> > support to display this information for all SQLSTATE class 23
> > (integrity constraint violation) errors which is not true as we forgot
> > to adhere to that in some parts of code.
> >
> > What do you think?  Anybody else has an opinion on whether to
> > back-patch this or not?
>
> As nobody except Chris complained about this so far, maybe no?
>

Fair enough, unless I see any other opinions, I will push this on Monday.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-03-23T02:52:00Z

    On Fri, Mar 20, 2020 at 12:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 19, 2020 at 3:34 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > >
> > > What do you think?  Anybody else has an opinion on whether to
> > > back-patch this or not?
> >
> > As nobody except Chris complained about this so far, maybe no?
> >
>
> Fair enough, unless I see any other opinions, I will push this on Monday.
>

Pushed.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com