Re: Minor inheritance/check bug: Inconsistent behavior

Bruce Momjian wrote:
On Sun, Jun 30, 2013 at 06:57:10AM +0000, Amit kapila wrote:
>> >> I have done the initial analysis and prepared a patch, don't know if
>> >> anything more I can do until
>> >> someone can give any suggestions to further proceed on this bug.
>
> > >So, I guess we never figured this out.
>
>> I can submit this bug-fix for next commitfest if there is no objection for doing so.
>> What is your opinion?

> Yes, good idea.

I had rebased the patch against head and added the test case to validate it.
I will upload this patch to commit fest.

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

Hi Amit.

I gone through the mail thread discussion regarding this issue and reviewed
you patch.

-- Patch get applied cleanly on Master branch
-- Make and Make Install fine
-- make check also running cleanly

In the patch code changes looks good to me.

This patch having two part:

1) Allowed TableOid system column in the CHECK constraint
2) Throw an error if other then TableOid system column in CHECK constraint.

I noticed that you added test coverage for 1) but the test coverage for 2)
is missing..

I added the test coverage for 2) in the attached patch.

Marking this as Ready for committer.

Regards,
Rushabh

On Sun, Sep 15, 2013 at 2:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

> Bruce Momjian wrote:
> On Sun, Jun 30, 2013 at 06:57:10AM +0000, Amit kapila wrote:
> >> >> I have done the initial analysis and prepared a patch, don't know if
> >> >> anything more I can do until
> >> >> someone can give any suggestions to further proceed on this bug.
> >
> > > >So, I guess we never figured this out.
> >
> >> I can submit this bug-fix for next commitfest if there is no objection
> for doing so.
> >> What is your opinion?
>
> > Yes, good idea.
>
> I had rebased the patch against head and added the test case to validate
> it.
> I will upload this patch to commit fest.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Rushabh Lathia
www.EnterpriseDB.com

On Tue, Sep 17, 2013 at 3:29 PM, Rushabh Lathia
<rushabh(dot)lathia(at)gmail(dot)com> wrote:
> Hi Amit.
>
> I gone through the mail thread discussion regarding this issue and reviewed
> you patch.
>
> -- Patch get applied cleanly on Master branch
> -- Make and Make Install fine
> -- make check also running cleanly
>
> In the patch code changes looks good to me.
>
> This patch having two part:
>
> 1) Allowed TableOid system column in the CHECK constraint
> 2) Throw an error if other then TableOid system column in CHECK constraint.
>
> I noticed that you added test coverage for 1) but the test coverage for 2)
> is missing..
Initially I thought of keeping the test for point-2 as well, but
later left it thinking it might not add much value for adding negative
test for this
scenario.
> I added the test coverage for 2) in the attached patch.
Thanks for adding new test.

> Marking this as Ready for committer.

Thanks a ton for reviewing the patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
>
>
> On Sun, Sep 15, 2013 at 2:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>
>> Bruce Momjian wrote:
>> On Sun, Jun 30, 2013 at 06:57:10AM +0000, Amit kapila wrote:
>> >> >> I have done the initial analysis and prepared a patch, don't know if
>> >> >> anything more I can do until
>> >> >> someone can give any suggestions to further proceed on this bug.
>> >
>> > > >So, I guess we never figured this out.
>> >
>> >> I can submit this bug-fix for next commitfest if there is no objection
>> >> for doing so.
>> >> What is your opinion?
>>
>> > Yes, good idea.
>>
>> I had rebased the patch against head and added the test case to validate
>> it.
>> I will upload this patch to commit fest.

>> 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.

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.

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
rhaas=# insert into foo values (1);
INSERT 16404 1
rhaas=# insert into foo values (1);
INSERT 16405 1
rhaas=# insert into foo values (1);
INSERT 16406 1
rhaas=# select oid, * from foo;
oid | a
-------+---
16404 | 1
16405 | 1
16406 | 1
(3 rows)

Just prohibiting that might be fine; it doesn't seem that useful
anyway. But if we're setting the table OID, maybe we should set the
OID too, and then just allow this.

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

On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas <robertmhaas(at)gmail(dot)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

> 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.

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

On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas <robertmhaas(at)gmail(dot)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

On Fri, Sep 20, 2013 at 3:41 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas <robertmhaas(at)gmail(dot)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

On Fri, Sep 20, 2013 at 5:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 20, 2013 at 3:41 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas <robertmhaas(at)gmail(dot)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 allowed in check constraint.

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

On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Handling for OID is not clear, shall we allow it or not in check constraint?
> In my current patch OID will not be allowed in check constraint.

I vote for allowing it.

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

On Fri, Sep 20, 2013 at 7:17 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Handling for OID is not clear, shall we allow it or not in check constraint?
>> In my current patch OID will not be allowed in check constraint.
>
> I vote for allowing it.

Okay, I shall send the updated patch by tomorrow.

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

On Fri, Sep 20, 2013 at 7:17 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Handling for OID is not clear, shall we allow it or not in check constraint?
>> In my current patch OID will not be allowed in check constraint.
>
> I vote for allowing it.

I had tried to allow for OID case, but setting OID in case of UPDATE
operation is bit tricky.
In ExecUpdate(), we need to set OID in new tuple by getting it from
old tuple, but we only have old tupleid in this function. Using old
tupleid, I can get tuple and then fetch OID from it to set in new
tuple as is done in heap_update(), but it seems bit more work and also
same has to be done in heap_update anyway unless we change signature
of heap_update().
For now, I had updated the patch for below points:
a. to set tableoid in Copy case
b. to set tableoid in Alter case
c. update the docs for Create Table Page. The other places like Alter
Table and Constraints page doesn't have any information related to
what is not allowed in
check constraints, so I left them as is.

I have not added new tests for Alter/Copy as they will test what
testcase for insert will test. However if you feel we should add test
for Alter/Copy/Update operation, then I will update the patch.

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