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

On Sun, Mar 1, 2020 at 10:10 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> Hi Chris,
>
> On Sun, Mar 1, 2020 at 4:34 AM Chris Bandy <bandy(dot)chris(at)gmail(dot)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