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

On Mon, Mar 2, 2020 at 9:39 AM Amit Langote <amitlangote09(at)gmail(dot)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