Re: Minor inheritance/check bug: Inconsistent behavior

On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> Marking this as Ready for committer.
>>>
>>> Thanks a ton for reviewing the patch.
>>
>> I rewrote the comments for this patch and fixed the incorrect
>> formatting in parse_relation.c.  It used spaces instead of tabs, which
>> is why if you look at the patch file you'll see that the alignment
>> looked off.  See new version, attached.
>    Thanks, Attached version looks better.
>
>> However, I have a few residual questions:
>>
>> 1. Does copy.c also need to be fixed?  I see that it also calls
>> ExecConstraints(), and I don't see it setting tableOid anywhere.  We
>> certainly want COPY and INSERT to behave the same way.
>
> I have missed it by confusing it with OID, as OID is set in COPY.
> I think along with COPY, it needs to set in ATRewriteTable() as well,
> because during rewrite also we check constraints.
>
> I will send an updated patch after point-2 is concluded.
>
>>
>> 2. Should constraints also allow access to the "oid" column?  Right
>> now you can do that but it doesn't seem to work, e.g.:
>>
>> rhaas=# create table foo (a int, check (oid::integer % 2 = 0)) with oids;
>> CREATE TABLE
>
> I have tried various combinations, it is giving error at my end.
> postgres=# create table tbl_with_oid(c1 int) With OIDS;
> CREATE TABLE
> postgres=# alter table tbl_with_oid add check (oid < 16403);
> ERROR:  system column "oid" reference in check constraint is invalid
> postgres=# alter table tbl_with_oid add check (oid =0);
> ERROR:  system column "oid" reference in check constraint is invalid
> postgres=# alter table tbl_with_oid add check (oid::integer %2 =0);
> ERROR:  system column "oid" reference in check constraint is invalid
> postgres=# create table foo (a int, check (oid::integer % 2 = 0)) with oids;
> ERROR:  system column "oid" reference in check constraint is invalid

I am sorry, I just realized after pressing send button that you want
to say the above point without considering above patch.

>> Just prohibiting that might be fine; it doesn't seem that useful
>> anyway.
>
> Currently only tableOid is allowed, other system columns should error
> out due to below check:
> +               /* In constraint check, no system column is allowed except tableOid */
> +               if (pstate->p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT &&
> +                       attnum < InvalidAttrNumber && attnum !=  TableOidAttributeNumber)
>
>> But if we're setting the table OID, maybe we should set the
>> OID too, and then just allow this.
>    It can be done for OID as well. I don't have any strong reason for
> either doing or not doing it, if you think it can be of use then we
> can add it.

One more thing, I think it will be better to update information in
docs, probably in Create Table page where CHECK constraints are
explained and where DDL constraints are explained at link:
http://www.postgresql.org/docs/devel/static/ddl-constraints.html

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com