Re: WITH CHECK and Column-Level Privileges

On Mon, Jan 12, 2015 at 05:16:40PM -0500, Stephen Frost wrote:
> Alright, here's an updated patch which doesn't return any detail if no
> values are visible or if only a partial key is visible.

I browsed this patch.  There's been no mention of foreign key constraints, but
ri_ReportViolation() deserves similar hardening.  If a user has only DELETE
privilege on a PK table, FK violation messages should not leak the PK values.
Modifications to the foreign side are less concerning, since the user will
often know the attempted value; still, I would lock down both sides.

Please add a comment explaining the safety of each row-emitting message you
haven't changed.  For example, the one in refresh_by_match_merge() is safe
because getting there requires MV ownership.

> --- a/src/backend/access/nbtree/nbtinsert.c
> +++ b/src/backend/access/nbtree/nbtinsert.c
> @@ -388,18 +388,30 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
>                      {
>                          Datum        values[INDEX_MAX_KEYS];
>                          bool        isnull[INDEX_MAX_KEYS];
> +                        char       *key_desc;
>  
>                          index_deform_tuple(itup, RelationGetDescr(rel),
>                                             values, isnull);
> -                        ereport(ERROR,
> -                                (errcode(ERRCODE_UNIQUE_VIOLATION),
> -                                 errmsg("duplicate key value violates unique constraint \"%s\"",
> -                                        RelationGetRelationName(rel)),
> -                                 errdetail("Key %s already exists.",
> -                                           BuildIndexValueDescription(rel,
> -                                                            values, isnull)),
> -                                 errtableconstraint(heapRel,
> -                                             RelationGetRelationName(rel))));
> +
> +                        key_desc = BuildIndexValueDescription(rel, values,
> +                                                              isnull);
> +
> +                        if (!strlen(key_desc))
> +                            ereport(ERROR,
> +                                    (errcode(ERRCODE_UNIQUE_VIOLATION),
> +                                     errmsg("duplicate key value violates unique constraint \"%s\"",
> +                                            RelationGetRelationName(rel)),
> +                                     errtableconstraint(heapRel,
> +                                                RelationGetRelationName(rel))));
> +                        else
> +                            ereport(ERROR,
> +                                    (errcode(ERRCODE_UNIQUE_VIOLATION),
> +                                     errmsg("duplicate key value violates unique constraint \"%s\"",
> +                                            RelationGetRelationName(rel)),
> +                                     errdetail("Key %s already exists.",
> +                                                key_desc),
> +                                     errtableconstraint(heapRel,
> +                                                RelationGetRelationName(rel))));

Instead of duplicating an entire ereport() to change whether you include an
errdetail, use "condition ? errdetail(...) : 0".