Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 20, 2013 at 7:17 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila amit.kapil...@gmail.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 sys_col_constr_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas robertmh...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 20, 2013 at 3:41 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 20, 2013 at 5:53 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Sep 20, 2013 at 3:41 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Fri, Sep 20, 2013 at 12:57 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas robertmh...@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 allowed in check constraint. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila amit.kapil...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 20, 2013 at 7:17 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila amit.kapil...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
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 sys_col_constr_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
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.kapil...@gmail.comwrote: Bruce Momjian wrote: On Sun, Jun 30, 2013 at 06:57:10AM +, 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Rushabh Lathia www.EnterpriseDB.com sys_col_constr_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Tue, Sep 17, 2013 at 3:29 PM, Rushabh Lathia rushabh.lat...@gmail.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.kapil...@gmail.com wrote: Bruce Momjian wrote: On Sun, Jun 30, 2013 at 06:57:10AM +, 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
Bruce Momjian wrote: On Sun, Jun 30, 2013 at 06:57:10AM +, 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 sys_col_constr_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Sun, Jun 30, 2013 at 06:57:10AM +, 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. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Saturday, June 29, 2013 4:58 AM Bruce Momjian wrote: On Sat, Jan 26, 2013 at 11:31:49AM +0530, Amit Kapila wrote: On Friday, January 25, 2013 8:36 PM Bruce Momjian wrote: On Fri, Sep 14, 2012 at 02:04:51PM +, Amit kapila wrote: On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote: On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila amit(dot)kapila(at)huawei(dot) com wrote: AFAICT during Update also, it doesn't contain useful. The only chance it would have contain something useful is when it goes for EvalPlanQual and then again comes to check for constraints. However these attributes get filled in heap_update much later. So now should the fix be that it returns an error for system column reference except for OID case? +1. 1. I think in this scenario the error for system column except for tableOID should be thrown at Create/Alter time. 2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside heap functions can be removed. But for now I have kept them as it is. Please find the Patch for bug-fix. If this is okay, I shall send you the test cases for same. Did we ever make any progress on this? 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? With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Sat, Jan 26, 2013 at 11:31:49AM +0530, Amit Kapila wrote: On Friday, January 25, 2013 8:36 PM Bruce Momjian wrote: On Fri, Sep 14, 2012 at 02:04:51PM +, Amit kapila wrote: On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote: On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila amit(dot)kapila(at)huawei(dot) com wrote: AFAICT during Update also, it doesn't contain useful. The only chance it would have contain something useful is when it goes for EvalPlanQual and then again comes to check for constraints. However these attributes get filled in heap_update much later. So now should the fix be that it returns an error for system column reference except for OID case? +1. 1. I think in this scenario the error for system column except for tableOID should be thrown at Create/Alter time. 2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside heap functions can be removed. But for now I have kept them as it is. Please find the Patch for bug-fix. If this is okay, I shall send you the test cases for same. Did we ever make any progress on this? 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. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Fri, Sep 14, 2012 at 02:04:51PM +, Amit kapila wrote: On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote: On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila amit(dot)kapila(at)huawei(dot) com wrote: AFAICT during Update also, it doesn't contain useful. The only chance it would have contain something useful is when it goes for EvalPlanQual and then again comes to check for constraints. However these attributes get filled in heap_update much later. So now should the fix be that it returns an error for system column reference except for OID case? +1. 1. I think in this scenario the error for system column except for tableOID should be thrown at Create/Alter time. 2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside heap functions can be removed. But for now I have kept them as it is. Please find the Patch for bug-fix. If this is okay, I shall send you the test cases for same. Did we ever make any progress on this? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Friday, January 25, 2013 8:36 PM Bruce Momjian wrote: On Fri, Sep 14, 2012 at 02:04:51PM +, Amit kapila wrote: On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote: On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila amit(dot)kapila(at)huawei(dot) com wrote: AFAICT during Update also, it doesn't contain useful. The only chance it would have contain something useful is when it goes for EvalPlanQual and then again comes to check for constraints. However these attributes get filled in heap_update much later. So now should the fix be that it returns an error for system column reference except for OID case? +1. 1. I think in this scenario the error for system column except for tableOID should be thrown at Create/Alter time. 2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside heap functions can be removed. But for now I have kept them as it is. Please find the Patch for bug-fix. If this is okay, I shall send you the test cases for same. Did we ever make any progress on this? 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. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
FW: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
Sorry, earlier sent below mail on hackers instead of bugs list. Just forwarding
the same here.
From: pgsql-hackers-ow...@postgresql.org [pgsql-hackers-ow...@postgresql.org]
on behalf of Amit kapila [amit.kap...@huawei.com]
Sent: Friday, September 14, 2012 7:34 PM
To: robertmh...@gmail.com
Cc: t...@sss.pgh.pa.us; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote:
On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila
amit(dot)kapila(at)huawei(dot)com wrote:
AFAICT during Update also, it doesn't contain useful. The only chance it
would have contain something useful is when it goes for EvalPlanQual and
then again comes to check for constraints. However these attributes get
filled in heap_update much later.
So now should the fix be that it returns an error for system column
reference except for OID case?
+1.
1. I think in this scenario the error for system column except for tableOID
should be thrown at Create/Alter time.
2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside
heap functions can be removed.
But for now I have kept them as it is.
Please find the Patch for bug-fix.
If this is okay, I shall send you the test cases for same.
With Regards,
Amit Kapila.
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 226,231 ExecInsert(TupleTableSlot *slot,
--- 226,235
}
else
{
+ /* Set the relation OID in tuple here only so that if OID is
used as part of constraint check then
+* it will be able to evaluate result of constraint correctly.*/
+ tuple-t_tableOid = RelationGetRelid(resultRelationDesc);
+
/*
* Check the constraints of the tuple
*/
***
*** 540,545 ExecUpdate(ItemPointer tupleid,
--- 544,553
}
else
{
+ /* Set the relation OID in tuple here only so that if OID is
used as part of constraint check then
+* it will be able to evaluate result of constraint
correctly.*/
+ tuple-t_tableOid = RelationGetRelid(resultRelationDesc);
+
/*
* Check the constraints of the tuple
*
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
***
*** 549,554 scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char
*colname,
--- 549,566
{
/* quick check to see if name could be a system column */
attnum = specialAttNum(colname);
+
+ /* In constraint check, no system column are allowed except
tableoid*/
+ if ((pstate-p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT)
+ (attnum InvalidAttrNumber)
+(attnum != TableOidAttributeNumber))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+ errmsg(system column \%s\ reference in check
constraint is invalid, colname),
+ parser_errposition(pstate, location)));
+ }
+
if (attnum != InvalidAttrNumber)
{
/* now check to see if column actually is defined */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote:
On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila
amit(dot)kapila(at)huawei(dot)com wrote:
AFAICT during Update also, it doesn't contain useful. The only chance it
would have contain something useful is when it goes for EvalPlanQual and
then again comes to check for constraints. However these attributes get
filled in heap_update much later.
So now should the fix be that it returns an error for system column
reference except for OID case?
+1.
1. I think in this scenario the error for system column except for tableOID
should be thrown at Create/Alter time.
2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside
heap functions can be removed.
But for now I have kept them as it is.
Please find the Patch for bug-fix.
If this is okay, I shall send you the test cases for same.
With Regards,
Amit Kapila.
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 226,231 ExecInsert(TupleTableSlot *slot,
--- 226,235
}
else
{
+ /* Set the relation OID in tuple here only so that if OID is
used as part of constraint check then
+* it will be able to evaluate result of constraint correctly.*/
+ tuple-t_tableOid = RelationGetRelid(resultRelationDesc);
+
/*
* Check the constraints of the tuple
*/
***
*** 540,545 ExecUpdate(ItemPointer tupleid,
--- 544,553
}
else
{
+ /* Set the relation OID in tuple here only so that if OID is
used as part of constraint check then
+* it will be able to evaluate result of constraint
correctly.*/
+ tuple-t_tableOid = RelationGetRelid(resultRelationDesc);
+
/*
* Check the constraints of the tuple
*
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
***
*** 549,554 scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char
*colname,
--- 549,566
{
/* quick check to see if name could be a system column */
attnum = specialAttNum(colname);
+
+ /* In constraint check, no system column are allowed except
tableoid*/
+ if ((pstate-p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT)
+ (attnum InvalidAttrNumber)
+(attnum != TableOidAttributeNumber))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+ errmsg(system column \%s\ reference in check
constraint is invalid, colname),
+ parser_errposition(pstate, location)));
+ }
+
if (attnum != InvalidAttrNumber)
{
/* now check to see if column actually is defined */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
