Re: Minor inheritance/check bug: Inconsistent behavior

On Fri, Sep 20, 2013 at 3:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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

Yes, agreed.  So, are you going to prepare a new version with
documentation and addressing the other points mentioned?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company