Thread: [PATCH] Add schema and table names to partition error
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
Attachment
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
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
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
Attachment
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
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
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
Attachment
> +\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
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
Attachment
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
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
Attachment
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
Attachment
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
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
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
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
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
Attachment
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
Attachment
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
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
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
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
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