Re: Minor inheritance/check bug: Inconsistent behavior

On Fri, Sep 20, 2013 at 5:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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?
  Handling for OID is not clear, shall we allow it or not in check constraint?  In my current patch OID will not be
allowedin check constraint.
 

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