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
Attachments:
sys_col_constr_v1.patchapplication/octet-stream; name=sys_col_constr_v1.patchDownload
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..2e2a745 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -247,6 +247,13 @@ ExecInsert(TupleTableSlot *slot,
else
{
/*
+ * Set the relation OID in tuple here so that if tableOID 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
*/
if (resultRelationDesc->rd_att->constr)
@@ -654,6 +661,13 @@ ExecUpdate(ItemPointer tupleid,
LockTupleMode lockmode;
/*
+ * Set the relation OID in tuple here so that if tableOID 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
*
* If we generate a new candidate tuple after EvalPlanQual testing, we
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 39922d3..46c04c1 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -551,6 +551,15 @@ scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char *colname,
{
/* quick check to see if name could be a system column */
attnum = specialAttNum(colname);
+ /* In constraint check, no system column is 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 */
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 2a63037..08e469c 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -127,6 +127,21 @@ INSERT INTO INSERT_TBL VALUES (null, null, null);
SELECT '' AS nine, * FROM INSERT_TBL;
--
+-- Check constraints on system columns
+--
+
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+
+DROP TABLE SYS_COL_CHECK_TBL;
+
+--
-- Check inheritance of defaults and constraints
--
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 18a5dd8..75a52e7 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -205,6 +205,23 @@ SELECT '' AS nine, * FROM INSERT_TBL;
(7 rows)
--
+-- Check constraints on system columns
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+ERROR: new row for relation "sys_col_check_tbl" violates check constraint "sys_col_check_tbl_check"
+DETAIL: Failing row contains (Olympia, Washington, t, 100).
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+ city | state | is_capital | altitude | tableoid
+---------+------------+------------+----------+-------------------
+ Seattle | Washington | f | 100 | sys_col_check_tbl
+(1 row)
+
+DROP TABLE SYS_COL_CHECK_TBL;
+--
-- Check inheritance of defaults and constraints
--
CREATE TABLE INSERT_CHILD (cx INT default 42,
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.kapila16@gmail.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
sys_col_constr_v2.patchapplication/octet-stream; name=sys_col_constr_v2.patchDownload
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..2e2a745 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -247,6 +247,13 @@ ExecInsert(TupleTableSlot *slot,
else
{
/*
+ * Set the relation OID in tuple here so that if tableOID 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
*/
if (resultRelationDesc->rd_att->constr)
@@ -654,6 +661,13 @@ ExecUpdate(ItemPointer tupleid,
LockTupleMode lockmode;
/*
+ * Set the relation OID in tuple here so that if tableOID 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
*
* If we generate a new candidate tuple after EvalPlanQual testing, we
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 39922d3..46c04c1 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -551,6 +551,15 @@ scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char *colname,
{
/* quick check to see if name could be a system column */
attnum = specialAttNum(colname);
+ /* In constraint check, no system column is 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 */
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 2a63037..16d38f6 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -127,6 +127,28 @@ INSERT INTO INSERT_TBL VALUES (null, null, null);
SELECT '' AS nine, * FROM INSERT_TBL;
--
+-- Check constraints on system columns
+--
+
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+
+DROP TABLE SYS_COL_CHECK_TBL;
+
+--
+-- Check constraints on system columns other then TableOid should return error
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
+
+--
-- Check inheritance of defaults and constraints
--
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 18a5dd8..2ffd263 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -205,6 +205,30 @@ SELECT '' AS nine, * FROM INSERT_TBL;
(7 rows)
--
+-- Check constraints on system columns
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+ERROR: new row for relation "sys_col_check_tbl" violates check constraint "sys_col_check_tbl_check"
+DETAIL: Failing row contains (Olympia, Washington, t, 100).
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+ city | state | is_capital | altitude | tableoid
+---------+------------+------------+----------+-------------------
+ Seattle | Washington | f | 100 | sys_col_check_tbl
+(1 row)
+
+DROP TABLE SYS_COL_CHECK_TBL;
+--
+-- Check constraints on system columns other then TableOid should return error
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
+ERROR: system column "ctid" reference in check constraint is invalid
+--
-- Check inheritance of defaults and constraints
--
CREATE TABLE INSERT_CHILD (cx INT default 42,
On Tue, Sep 17, 2013 at 3:29 PM, Rushabh Lathia
<rushabh.lathia@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 cleanlyIn 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.kapila16@gmail.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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
sys_col_constr_v3.patchapplication/octet-stream; name=sys_col_constr_v3.patchDownload
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..189df71 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -247,6 +247,12 @@ ExecInsert(TupleTableSlot *slot,
else
{
/*
+ * Constraints might reference the tableoid column, so initialize
+ * t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
+
+ /*
* Check the constraints of the tuple
*/
if (resultRelationDesc->rd_att->constr)
@@ -654,6 +660,12 @@ ExecUpdate(ItemPointer tupleid,
LockTupleMode lockmode;
/*
+ * Constraints might reference the tableoid column, so initialize
+ * t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
+
+ /*
* Check the constraints of the tuple
*
* If we generate a new candidate tuple after EvalPlanQual testing, we
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 39922d3..5f46913 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -551,6 +551,16 @@ scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char *colname,
{
/* quick check to see if name could be a system column */
attnum = specialAttNum(colname);
+
+ /* In constraint check, no system column is 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 */
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 2a63037..16d38f6 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -127,6 +127,28 @@ INSERT INTO INSERT_TBL VALUES (null, null, null);
SELECT '' AS nine, * FROM INSERT_TBL;
--
+-- Check constraints on system columns
+--
+
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+
+DROP TABLE SYS_COL_CHECK_TBL;
+
+--
+-- Check constraints on system columns other then TableOid should return error
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
+
+--
-- Check inheritance of defaults and constraints
--
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 18a5dd8..2ffd263 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -205,6 +205,30 @@ SELECT '' AS nine, * FROM INSERT_TBL;
(7 rows)
--
+-- Check constraints on system columns
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+ERROR: new row for relation "sys_col_check_tbl" violates check constraint "sys_col_check_tbl_check"
+DETAIL: Failing row contains (Olympia, Washington, t, 100).
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+ city | state | is_capital | altitude | tableoid
+---------+------------+------------+----------+-------------------
+ Seattle | Washington | f | 100 | sys_col_check_tbl
+(1 row)
+
+DROP TABLE SYS_COL_CHECK_TBL;
+--
+-- Check constraints on system columns other then TableOid should return error
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
+ERROR: system column "ctid" reference in check constraint is invalid
+--
-- Check inheritance of defaults and constraints
--
CREATE TABLE INSERT_CHILD (cx INT default 42,
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
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
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 TABLEI 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
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 TABLEI 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 invalidI 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
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 TABLEI 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 invalidI 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.htmlYes, 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
On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila <amit.kapila16@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
On Fri, Sep 20, 2013 at 7:17 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila <amit.kapila16@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
On Fri, Sep 20, 2013 at 7:17 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Sep 20, 2013 at 9:43 AM, Amit Kapila <amit.kapila16@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
Attachments:
sys_col_constr_v4.patchapplication/octet-stream; name=sys_col_constr_v4.patchDownload
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 26eca67..15c6a9a 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -430,7 +430,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
<para>
Currently, <literal>CHECK</literal> expressions cannot contain
subqueries nor refer to variables other than columns of the
- current row.
+ current row or system column tableoid.
</para>
<para>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 31819cc..6b20144 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2217,6 +2217,12 @@ CopyFrom(CopyState cstate)
if (loaded_oid != InvalidOid)
HeapTupleSetOid(tuple, loaded_oid);
+ /*
+ * Constraints might reference the tableoid column, so initialize
+ * t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
+
/* Triggers and stuff need to be invoked in query context. */
MemoryContextSwitchTo(oldcontext);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index adc74dd..8839f98 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3857,6 +3857,12 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
/* Preserve OID, if any */
if (newTupDesc->tdhasoid)
HeapTupleSetOid(tuple, tupOid);
+
+ /*
+ * Constraints might reference the tableoid column, so initialize
+ * t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(oldrel);
}
/* Now check any constraints on the possibly-changed tuple */
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..189df71 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -247,6 +247,12 @@ ExecInsert(TupleTableSlot *slot,
else
{
/*
+ * Constraints might reference the tableoid column, so initialize
+ * t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
+
+ /*
* Check the constraints of the tuple
*/
if (resultRelationDesc->rd_att->constr)
@@ -654,6 +660,12 @@ ExecUpdate(ItemPointer tupleid,
LockTupleMode lockmode;
/*
+ * Constraints might reference the tableoid column, so initialize
+ * t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
+
+ /*
* Check the constraints of the tuple
*
* If we generate a new candidate tuple after EvalPlanQual testing, we
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 39922d3..5f46913 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -551,6 +551,16 @@ scanRTEForColumn(ParseState *pstate, RangeTblEntry *rte, char *colname,
{
/* quick check to see if name could be a system column */
attnum = specialAttNum(colname);
+
+ /* In constraint check, no system column is 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 */
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 2a63037..16d38f6 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -127,6 +127,28 @@ INSERT INTO INSERT_TBL VALUES (null, null, null);
SELECT '' AS nine, * FROM INSERT_TBL;
--
+-- Check constraints on system columns
+--
+
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+
+DROP TABLE SYS_COL_CHECK_TBL;
+
+--
+-- Check constraints on system columns other then TableOid should return error
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
+
+--
-- Check inheritance of defaults and constraints
--
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 18a5dd8..2ffd263 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -205,6 +205,30 @@ SELECT '' AS nine, * FROM INSERT_TBL;
(7 rows)
--
+-- Check constraints on system columns
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND tableoid::regclass::text = 'sys_col_check_tbl')));
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Seattle', 'Washington', false, 100);
+INSERT INTO SYS_COL_CHECK_TBL VALUES ('Olympia', 'Washington', true, 100);
+ERROR: new row for relation "sys_col_check_tbl" violates check constraint "sys_col_check_tbl_check"
+DETAIL: Failing row contains (Olympia, Washington, t, 100).
+SELECT *, tableoid::regclass::text FROM SYS_COL_CHECK_TBL;
+ city | state | is_capital | altitude | tableoid
+---------+------------+------------+----------+-------------------
+ Seattle | Washington | f | 100 | sys_col_check_tbl
+(1 row)
+
+DROP TABLE SYS_COL_CHECK_TBL;
+--
+-- Check constraints on system columns other then TableOid should return error
+--
+CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
+ altitude int,
+ CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
+ERROR: system column "ctid" reference in check constraint is invalid
+--
-- Check inheritance of defaults and constraints
--
CREATE TABLE INSERT_CHILD (cx INT default 42,