Re: [PATCH] Add schema and table names to partition error - Mailing list pgsql-hackers

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