Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()
Hi ALL,
Need your suggestions.
initially_valid flag is added to make column constraint valid. (commit : /messages/by-id/E1Q2Ak9-0006hK-M4@gemulon.postgresql.org)
IFAICU, initially_valid and skip_validation values are mutually exclusive at constraint creation(ref: gram.y), unless it set explicitly.
Can we pass initially_valid instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints(), as shown below?
==========================================================================================
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7d7d062..04c4f8f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2349,7 +2349,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
constrOid =
- StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
+ StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
is_local ? 0 : 1, cdef->is_no_inherit, is_internal);numchecks++;
==========================================================================================
This will make code more readable & in my case this could enable to skip validation of existing data as well as mark check constraint valid, when we have assurance that modified/added constraint are valid.
Comments? Thoughts?
Regards,
Amul Sul
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reference msg id not found: 862791678.12260510.1449132763399.JavaMail.yahoo.ref@mail.yahoo.com
Hi Amul!
On 2015/12/03 17:52, amul sul wrote:
Hi ALL,
Need your suggestions.
initially_valid flag is added to make column constraint valid. (commit : /messages/by-id/E1Q2Ak9-0006hK-M4@gemulon.postgresql.org)IFAICU, initially_valid and skip_validation values are mutually exclusive at constraint creation(ref: gram.y), unless it set explicitly.
Can we pass initially_valid instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints(), as shown below?
...
This will make code more readable & in my case this could enable to skip validation of existing data as well as mark check constraint valid, when we have assurance that modified/added constraint are valid.
Comments? Thoughts?
Especially from a readability standpoint, I think using skip_validation
may be more apt. Why - the corresponding parameter of StoreRelCheck()
dictates what's stored in pg_constraint.convalidated. So, if
skip_validation is 'true' because user specified the constraint NOT VALID,
StoreRelCheck() will store the constraint with convalidated as 'false',
because, well, user wishes to "skip" the validation for existing rows in
the table and until a constraint has been verified for all rows in the
table, it cannot be marked valid. The user will have to separately
validate the constraint by issuing a ALTER TABLE VALIDATE CONSTRAINT
command at a time of their choosing.
OTOH, if NOT VALID was not specified, validation will not be skipped -
skip_validation would be 'false', so the constraint would be stored as
valid and added to the list of constraints to be atomically verified in
the last phase of ALTER TABLE processing.
Does that make sense?
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Amit,
Thanks for prompt response.
On Thursday, 3 December 2015 4:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Especially from a readability standpoint, I think using skip_validation may be more apt.
Why - the corresponding parameter of StoreRelCheck() dictates what's stored in pg_constraint.convalidated.
Why not? won't initially_valid flag serve same purpose ?
So, if skip_validation is 'true' because user specified the constraint NOT VALID,
StoreRelCheck() will store the constraint with convalidated as 'false'
I guess thats was added before initially_valid flag. As I said, in normal case gram.y take care of skip_validation & initially_valid values, if one is 'true' other will be 'false'.
The user will have to separately validate the constraint by issuing a ALTER TABLE VALIDATE CONSTRAINT
command at a time of their choosing.
This could be time consuming operation for big table, If I am pretty much sure that my constraint will be valid, simply I could set both flag(initially_valid & skip_validation) to true.
Regards,
Amul Sul
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/3/15 12:44 PM, amul sul wrote:
On Thursday, 3 December 2015 4:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
The user will have to separately validate the constraint by issuing a ALTER TABLE VALIDATE CONSTRAINT
command at a time of their choosing.This could be time consuming operation for big table, If I am pretty much sure that my constraint will be valid, simply I could set both flag(initially_valid & skip_validation) to true.
I'm confused here. It sounds like you're suggesting an SQL level
feature, but you're really focused on a single line of code for some
reason. Could you take a step back and explain the high level picture
of what you're trying to achieve?
.m
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/12/03 20:44, amul sul wrote:
On Thursday, 3 December 2015 4:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Especially from a readability standpoint, I think using skip_validation may be more apt.
Why - the corresponding parameter of StoreRelCheck() dictates what's stored in pg_constraint.convalidated.Why not? won't initially_valid flag serve same purpose ?
Yes it could, but IMO, it wouldn't be a readability improvement. As I
said, you could think of the variable/argument as delivering whether the
clause is marked NOT VALID or not. Also, see below.
So, if skip_validation is 'true' because user specified the constraint NOT VALID,
StoreRelCheck() will store the constraint with convalidated as 'false'I guess thats was added before initially_valid flag. As I said, in normal case gram.y take care of skip_validation & initially_valid values, if one is 'true' other will be 'false'.
The user will have to separately validate the constraint by issuing a ALTER TABLE VALIDATE CONSTRAINT
command at a time of their choosing.This could be time consuming operation for big table, If I am pretty much sure that my constraint will be valid, simply I could set both flag(initially_valid & skip_validation) to true.
There is currently no support for adding a constraint after-the-fact (that
is, using ALTER TABLE) and marking it valid without actually verifying it
by scanning the table. As Marko points out that would be actually a new
SQL-level feature that requires much more than changing that line.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
let me put it in other words, is there any harm use of initially_valid instead of !skip_validation?
Earlier to commit I mentioned in my first post, there is only one flag, IMO i.e. skip_validation, which are serving both purpose, setting pg_constraint.convalidated & decide to skip or validate existing data.
Now, if we have two flag, which can separately use for there respective purpose, then why do you think that it is not readable?
As Marko points out that would be actually a new
SQL-level feature that requires much more than changing that line.
May be yes.
Regards, Amul Sul
On Friday, 4 December 2015 6:21 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2015/12/03 20:44, amul sul wrote:
On Thursday, 3 December 2015 4:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Especially from a readability standpoint, I think using skip_validation may be more apt.
Why - the corresponding parameter of StoreRelCheck() dictates what's stored in pg_constraint.convalidated.Why not? won't initially_valid flag serve same purpose ?
Yes it could, but IMO, it wouldn't be a readability improvement. As I
said, you could think of the variable/argument as delivering whether the
clause is marked NOT VALID or not. Also, see below.
So, if skip_validation is 'true' because user specified the constraint NOT VALID,
StoreRelCheck() will store the constraint with convalidated as 'false'I guess thats was added before initially_valid flag. As I said, in normal case gram.y take care of skip_validation & initially_valid values, if one is 'true' other will be 'false'.
The user will have to separately validate the constraint by issuing a ALTER TABLE VALIDATE CONSTRAINT
command at a time of their choosing.This could be time consuming operation for big table, If I am pretty much sure that my constraint will be valid, simply I could set both flag(initially_valid & skip_validation) to true.
There is currently no support for adding a constraint after-the-fact (that
is, using ALTER TABLE) and marking it valid without actually verifying it
by scanning the table. As Marko points out that would be actually a new
SQL-level feature that requires much more than changing that line.
Thanks,
Amit
--
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
On Thu, Dec 3, 2015 at 3:52 AM, amul sul <sul_amul@yahoo.co.in> wrote:
Hi ALL,
Need your suggestions.
initially_valid flag is added to make column constraint valid. (commit : /messages/by-id/E1Q2Ak9-0006hK-M4@gemulon.postgresql.org)IFAICU, initially_valid and skip_validation values are mutually exclusive at constraint creation(ref: gram.y), unless it set explicitly.
Can we pass initially_valid instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints(), as shown below?
The comments say this:
* If skip_validation is true then we skip checking that the existing rows
* in the table satisfy the constraint, and just install the catalog entries
* for the constraint. A new FK constraint is marked as valid iff
* initially_valid is true. (Usually skip_validation and initially_valid
* are inverses, but we can set both true if the table is known empty.)
These comments are accurate. If you create a FK constraint as not
valid, the system decides that it's really valid after all, but if you
add a CHECK constraint as not valid, the system just believes you:
rhaas=# create table foo (a serial primary key);
CREATE TABLE
rhaas=# create table bar (a int, foreign key (a) references foo (a)
not valid, check (a != 0) not valid);
CREATE TABLE
rhaas=# \d bar
Table "public.bar"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Check constraints:
"bar_a_check" CHECK (a <> 0) NOT VALID
Foreign-key constraints:
"bar_a_fkey" FOREIGN KEY (a) REFERENCES foo(a)
I suspect this is an oversight. We allowed FOREIGN KEY constraints to
be not valid in 722bf7017bbe796decc79c1fde03e7a83dae9ada by Simon
Riggs, but didn't add allow it for CHECK constraints until Alvaro's
commit of 897795240cfaaed724af2f53ed2c50c9862f951f a few months later.
My guess is that there's no reason for these not to behave in the same
way, but they don't. Amul's proposed one-liner might be one part of
actually fixing that, but it wouldn't be enough by itself: you'd also
need to teach transformCreateStmt to set the initially_valid flag to
true, maybe by adding a new function transformCheckConstraints or so.
--
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 2015/12/09 5:50, Robert Haas wrote:
On Thu, Dec 3, 2015 at 3:52 AM, amul sul <sul_amul@yahoo.co.in> wrote:
Can we pass initially_valid instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints(), as shown below?
The comments say this:
* If skip_validation is true then we skip checking that the existing rows
* in the table satisfy the constraint, and just install the catalog entries
* for the constraint. A new FK constraint is marked as valid iff
* initially_valid is true. (Usually skip_validation and initially_valid
* are inverses, but we can set both true if the table is known empty.)These comments are accurate. If you create a FK constraint as not
valid, the system decides that it's really valid after all, but if you
add a CHECK constraint as not valid, the system just believes you:rhaas=# create table foo (a serial primary key);
CREATE TABLE
rhaas=# create table bar (a int, foreign key (a) references foo (a)
not valid, check (a != 0) not valid);
CREATE TABLE
rhaas=# \d bar
Table "public.bar"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Check constraints:
"bar_a_check" CHECK (a <> 0) NOT VALID
Foreign-key constraints:
"bar_a_fkey" FOREIGN KEY (a) REFERENCES foo(a)
Didn't realize that marking constraints NOT VALID during table creation
was syntactically possible. Now I see that the same grammar rule for table
constraints is used for both create table and alter table add constraint.
I suspect this is an oversight. We allowed FOREIGN KEY constraints to
be not valid in 722bf7017bbe796decc79c1fde03e7a83dae9ada by Simon
Riggs, but didn't add allow it for CHECK constraints until Alvaro's
commit of 897795240cfaaed724af2f53ed2c50c9862f951f a few months later.
My guess is that there's no reason for these not to behave in the same
way, but they don't. Amul's proposed one-liner might be one part of
actually fixing that, but it wouldn't be enough by itself: you'd also
need to teach transformCreateStmt to set the initially_valid flag to
true, maybe by adding a new function transformCheckConstraints or so.
So, any NOT VALID specification for a FK constraint is effectively
overridden in transformFKConstraints() at table creation time but the same
doesn't happen for CHECK constraints. I agree that that could be fixed,
then as you say, Amul's one-liner would make sense.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/12/09 11:19, Amit Langote wrote:
On 2015/12/09 5:50, Robert Haas wrote:
I suspect this is an oversight. We allowed FOREIGN KEY constraints to
be not valid in 722bf7017bbe796decc79c1fde03e7a83dae9ada by Simon
Riggs, but didn't add allow it for CHECK constraints until Alvaro's
commit of 897795240cfaaed724af2f53ed2c50c9862f951f a few months later.
My guess is that there's no reason for these not to behave in the same
way, but they don't. Amul's proposed one-liner might be one part of
actually fixing that, but it wouldn't be enough by itself: you'd also
need to teach transformCreateStmt to set the initially_valid flag to
true, maybe by adding a new function transformCheckConstraints or so.So, any NOT VALID specification for a FK constraint is effectively
overridden in transformFKConstraints() at table creation time but the same
doesn't happen for CHECK constraints. I agree that that could be fixed,
then as you say, Amul's one-liner would make sense.
So, how about attached?
I think it may be enough to flip initially_valid to true in
transformTableConstraint() when in a CREATE TABLE context.
Regarding Amul's proposed change, there arises one minor inconsistency.
StoreRelCheck() is called in two places - AddRelationNewConstraints(),
where we can safely change from passing the value of !skip_validation to
that of initially_valid and StoreConstraints(), where we cannot because
CookedConstraint is used which doesn't have the initially_valid field.
Nevertheless, attached patch includes the former.
Thoughts?
Thanks,
Amit
Attachments:
check-con-no-novalid-1.patchtext/x-diff; name=check-con-no-novalid-1.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7d7d062..04c4f8f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2349,7 +2349,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
constrOid =
- StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
+ StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
is_local ? 0 : 1, cdef->is_no_inherit, is_internal);
numchecks++;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 344a40c..ce45804 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -687,6 +687,10 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
break;
case CONSTR_CHECK:
+ /* Is this better done in a transformCheckConstraint? */
+ if (!cxt->isalter)
+ constraint->initially_valid = true;
+
cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
break;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 61669b6..c91342f 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -187,7 +187,7 @@ Table "public.constraint_rename_test"
b | integer |
c | integer |
Check constraints:
- "con1" CHECK (a > 0)
+ "con1" CHECK (a > 0) NOT VALID
CREATE TABLE constraint_rename_test2 (a int CONSTRAINT con1 CHECK (a > 0), d int) INHERITS (constraint_rename_test);
NOTICE: merging column "a" with inherited definition
@@ -217,7 +217,7 @@ Table "public.constraint_rename_test"
b | integer |
c | integer |
Check constraints:
- "con1foo" CHECK (a > 0)
+ "con1foo" CHECK (a > 0) NOT VALID
Number of child tables: 1 (Use \d+ to list them.)
\d constraint_rename_test2
@@ -243,7 +243,7 @@ Table "public.constraint_rename_test"
b | integer |
c | integer |
Check constraints:
- "con1foo" CHECK (a > 0)
+ "con1foo" CHECK (a > 0) NOT VALID
"con2bar" CHECK (b > 0) NO INHERIT
Number of child tables: 1 (Use \d+ to list them.)
@@ -271,7 +271,7 @@ Table "public.constraint_rename_test"
Indexes:
"con3foo" PRIMARY KEY, btree (a)
Check constraints:
- "con1foo" CHECK (a > 0)
+ "con1foo" CHECK (a > 0) NOT VALID
"con2bar" CHECK (b > 0) NO INHERIT
Number of child tables: 1 (Use \d+ to list them.)
@@ -1820,7 +1820,7 @@ CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
a | double precision |
b | double precision |
Check constraints:
- "test_inh_check_a_check" CHECK (a > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a > 10.2::double precision) NOT VALID
Number of child tables: 1 (Use \d+ to list them.)
\d test_inh_check_child
@@ -1851,7 +1851,7 @@ ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
a | numeric |
b | double precision |
Check constraints:
- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) NOT VALID
Number of child tables: 1 (Use \d+ to list them.)
\d test_inh_check_child
@@ -1861,7 +1861,7 @@ Number of child tables: 1 (Use \d+ to list them.)
a | numeric |
b | double precision |
Check constraints:
- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) NOT VALID
Inherits: test_inh_check
select relname, conname, coninhcount, conislocal, connoinherit
@@ -1889,7 +1889,7 @@ NOTICE: merging constraint "bmerged" with inherited definition
Check constraints:
"bmerged" CHECK (b > 1::double precision)
"bnoinherit" CHECK (b > 100::double precision) NO INHERIT
- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) NOT VALID
Number of child tables: 1 (Use \d+ to list them.)
\d test_inh_check_child
@@ -1901,7 +1901,7 @@ Number of child tables: 1 (Use \d+ to list them.)
Check constraints:
"blocal" CHECK (b < 1000::double precision)
"bmerged" CHECK (b > 1::double precision)
- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) NOT VALID
Inherits: test_inh_check
select relname, conname, coninhcount, conislocal, connoinherit
@@ -1929,7 +1929,7 @@ Table "public.test_inh_check"
Check constraints:
"bmerged" CHECK (b::double precision > 1::double precision)
"bnoinherit" CHECK (b::double precision > 100::double precision) NO INHERIT
- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) NOT VALID
Number of child tables: 1 (Use \d+ to list them.)
\d test_inh_check_child
@@ -1941,7 +1941,7 @@ Table "public.test_inh_check_child"
Check constraints:
"blocal" CHECK (b::double precision < 1000::double precision)
"bmerged" CHECK (b::double precision > 1::double precision)
- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+ "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) NOT VALID
Inherits: test_inh_check
select relname, conname, coninhcount, conislocal, connoinherit
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
index 97edde1..55b3706 100644
--- a/src/test/regress/expected/create_table_like.out
+++ b/src/test/regress/expected/create_table_like.out
@@ -171,7 +171,7 @@ NOTICE: merging column "a" with inherited definition
c | text | | external | | C
Check constraints:
"ctlt1_a_check" CHECK (length(a) > 2)
- "ctlt3_a_check" CHECK (length(a) < 5)
+ "ctlt3_a_check" CHECK (length(a) < 5) NOT VALID
Inherits: ctlt1
SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt13_like'::regclass;
@@ -192,7 +192,7 @@ Indexes:
"ctlt_all_b_idx" btree (b)
"ctlt_all_expr_idx" btree ((a || b))
Check constraints:
- "ctlt1_a_check" CHECK (length(a) > 2)
+ "ctlt1_a_check" CHECK (length(a) > 2) NOT VALID
SELECT c.relname, objsubid, description FROM pg_description, pg_index i, pg_class c WHERE classoid = 'pg_class'::regclass AND objoid = i.indexrelid AND c.oid = i.indexrelid AND i.indrelid = 'ctlt_all'::regclass ORDER BY c.relname, objsubid;
relname | objsubid | description
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 73c02bb..4596dbc 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -709,7 +709,7 @@ COMMENT ON COLUMN ft1.c1 IS 'ft1.c1';
c2 | text | | (param2 'val2', param3 'val3') | extended | |
c3 | date | | | plain | |
Check constraints:
- "ft1_c2_check" CHECK (c2 <> ''::text)
+ "ft1_c2_check" CHECK (c2 <> ''::text) NOT VALID
"ft1_c3_check" CHECK (c3 >= '01-01-1994'::date AND c3 <= '01-31-1994'::date)
Server: s0
FDW Options: (delimiter ',', quote '"', "be quoted" 'value')
@@ -771,7 +771,7 @@ ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 SET STORAGE PLAIN;
c9 | integer | | | plain | |
c10 | integer | | (p1 'v1') | plain | |
Check constraints:
- "ft1_c2_check" CHECK (c2 <> ''::text)
+ "ft1_c2_check" CHECK (c2 <> ''::text) NOT VALID
"ft1_c3_check" CHECK (c3 >= '01-01-1994'::date AND c3 <= '01-31-1994'::date)
Server: s0
FDW Options: (delimiter ',', quote '"', "be quoted" 'value')
@@ -820,7 +820,7 @@ ALTER FOREIGN TABLE foreign_schema.ft1 RENAME TO foreign_table_1;
c8 | text | | (p2 'V2')
c10 | integer | | (p1 'v1')
Check constraints:
- "ft1_c2_check" CHECK (c2 <> ''::text)
+ "ft1_c2_check" CHECK (c2 <> ''::text) NOT VALID
"ft1_c3_check" CHECK (c3 >= '01-01-1994'::date AND c3 <= '01-31-1994'::date)
Server: s0
FDW Options: (quote '~', "be quoted" 'value', escape '@')
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 89b6c1c..7853e41 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -849,7 +849,7 @@ alter table pp1 add column a1 int check (a1 > 0);
f3 | integer |
a1 | integer |
Check constraints:
- "pp1_a1_check" CHECK (a1 > 0)
+ "pp1_a1_check" CHECK (a1 > 0) NOT VALID
Inherits: pp1
create table cc2(f4 float) inherits(pp1,cc1);
@@ -884,7 +884,7 @@ NOTICE: merging constraint "pp1_a2_check" with inherited definition
a2 | integer |
Check constraints:
"pp1_a1_check" CHECK (a1 > 0)
- "pp1_a2_check" CHECK (a2 > 0)
+ "pp1_a2_check" CHECK (a2 > 0) NOT VALID
Inherits: pp1,
cc1
On Wednesday, 9 December 2015 12:55 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Thoughts?
Wondering, have you notice failed regression tests?
I have tried with new transformCheckConstraints() function & there will be small fix in gram.y.
Have look into attached patch & please share your thoughts and/or suggestions.
Thanks,
Amul Sul
Attachments:
transformCheckConstraints-function-to-overrid-not-valid.patchapplication/octet-streamDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7d7d062..04c4f8f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2349,7 +2349,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
constrOid =
- StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
+ StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
is_local ? 0 : 1, cdef->is_no_inherit, is_internal);
numchecks++;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index fba91d5..8fdc442 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3077,6 +3077,8 @@ ColConstraintElem:
n->is_no_inherit = $5;
n->raw_expr = $3;
n->cooked_expr = NULL;
+ n->skip_validation = false;
+ n->initially_valid = true;
$$ = (Node *)n;
}
| DEFAULT b_expr
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 344a40c..df8b04f 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -120,6 +120,8 @@ static IndexStmt *transformIndexConstraint(Constraint *constraint,
static void transformFKConstraints(CreateStmtContext *cxt,
bool skipValidation,
bool isAddConstraint);
+static void transformCheckConstraints(CreateStmtContext *cxt,
+ bool skipValidation);
static void transformConstraintAttrs(CreateStmtContext *cxt,
List *constraintList);
static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
@@ -320,6 +322,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
transformFKConstraints(&cxt, true, false);
/*
+ * Postprocess check constraints.
+ */
+ transformCheckConstraints(&cxt, true);
+
+ /*
* Output results.
*/
stmt->tableElts = cxt.columns;
@@ -1915,6 +1922,32 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
}
/*
+ * transformCheckConstraints
+ * handle CHECK constraints
+ */
+static void
+transformCheckConstraints(CreateStmtContext *cxt, bool skipValidation)
+{
+ ListCell *ckclist;
+
+ if (cxt->ckconstraints == NIL)
+ return;
+ /*
+ * If CREATE TABLE we can safely skip validation of check
+ * constraints, and nonetheless mark them valid.
+ * (This will override any user-supplied NOT VALID flag.)
+ */
+ if (skipValidation)
+ foreach(ckclist, cxt->ckconstraints)
+ {
+ Constraint *constraint = (Constraint *) lfirst(ckclist);
+
+ constraint->skip_validation = true;
+ constraint->initially_valid = true;
+ }
+}
+
+/*
* transformFKConstraints
* handle FOREIGN KEY constraints
*/
On Wednesday, 9 December 2015, amul sul <sul_amul@yahoo.co.in> wrote:
On Wednesday, 9 December 2015 12:55 PM, Amit Langote <
Langote_Amit_f8@lab.ntt.co.jp <javascript:;>> wrote:
Thoughts?
Wondering, have you notice failed regression tests?
I did, I couldn't send an updated patch today before leaving for the day.
I have tried with new transformCheckConstraints() function & there will be
small fix in gram.y.Have look into attached patch & please share your thoughts and/or
suggestions.
Will take a look.
Thanks,
Amit
On 2015/12/09 18:07, amul sul wrote:
On Wednesday, 9 December 2015 12:55 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Thoughts?
Wondering, have you notice failed regression tests?
Here is the updated patch.
I have tried with new transformCheckConstraints() function & there will be small fix in gram.y.
Have look into attached patch & please share your thoughts and/or suggestions.
The transformCheckConstraints approach may be better after all.
By the way,
@@ -1915,6 +1922,32 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
...
+ if (skipValidation) + foreach(ckclist, cxt->ckconstraints) + { + Constraint *constraint = (Constraint *) lfirst(ckclist); + + constraint->skip_validation = true; + constraint->initially_valid = true; + }
You forgot to put braces around the if block.
Thanks,
Amit
Attachments:
check-con-no-novalid-2.patchtext/x-diff; name=check-con-no-novalid-2.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7d7d062..04c4f8f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2349,7 +2349,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
constrOid =
- StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
+ StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
is_local ? 0 : 1, cdef->is_no_inherit, is_internal);
numchecks++;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 344a40c..f0c3e76 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -562,6 +562,11 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
break;
case CONSTR_CHECK:
+ /*
+ * When being added as part of a column definition, the
+ * following always holds.
+ */
+ constraint->initially_valid = true;
cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
break;
@@ -687,6 +692,10 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
break;
case CONSTR_CHECK:
+ /* Is this better done in a transformCheckConstraint? */
+ if (!cxt->isalter)
+ constraint->initially_valid = true;
+
cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
break;
@@ -935,6 +944,7 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
n->conname = pstrdup(ccname);
n->raw_expr = NULL;
n->cooked_expr = nodeToString(ccbin_node);
+ n->initially_valid = true;
cxt->ckconstraints = lappend(cxt->ckconstraints, n);
/* Copy comment on constraint */
On Thursday, 10 December 2015 8:22 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
You forgot to put braces around the if block.
Does this really required?
Regards,
Amul Sul
--
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
On 2015/12/10 13:12, amul sul wrote:
On Thursday, 10 December 2015 8:22 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
You forgot to put braces around the if block.
Does this really required?
If nothing else, for consistency with surrounding code.
Take a look at a similar code block in transformFKConstraints
(parse_utilcmd.c: line 1935), or transformIndexConstraint
(parse_utilcmd.c: line 1761).
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015/12/10 13:38, Amit Langote wrote:
Take a look at a similar code block in transformFKConstraints
(parse_utilcmd.c: line 1935), or transformIndexConstraint
(parse_utilcmd.c: line 1761).
Oops, forget the second one.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thursday, 10 December 2015 10:13 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2015/12/10 13:38, Amit Langote wrote:
Take a look at a similar code block in transformFKConstraints
(parse_utilcmd.c: line 1935), or transformIndexConstraint
(parse_utilcmd.c: line 1761).
Oops, forget the second one.
No issue, first one make sense.
Updated patch is attached.
Thanks & Regards,
Amul Sul
Attachments:
transformCheckConstraints-function-to-overrid-not-valid_V2.patchapplication/octet-streamDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7d7d062..04c4f8f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2349,7 +2349,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
constrOid =
- StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
+ StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
is_local ? 0 : 1, cdef->is_no_inherit, is_internal);
numchecks++;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index fba91d5..8fdc442 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3077,6 +3077,8 @@ ColConstraintElem:
n->is_no_inherit = $5;
n->raw_expr = $3;
n->cooked_expr = NULL;
+ n->skip_validation = false;
+ n->initially_valid = true;
$$ = (Node *)n;
}
| DEFAULT b_expr
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 344a40c..566597b 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -120,6 +120,8 @@ static IndexStmt *transformIndexConstraint(Constraint *constraint,
static void transformFKConstraints(CreateStmtContext *cxt,
bool skipValidation,
bool isAddConstraint);
+static void transformCheckConstraints(CreateStmtContext *cxt,
+ bool skipValidation);
static void transformConstraintAttrs(CreateStmtContext *cxt,
List *constraintList);
static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
@@ -320,6 +322,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
transformFKConstraints(&cxt, true, false);
/*
+ * Postprocess check constraints.
+ */
+ transformCheckConstraints(&cxt, true);
+
+ /*
* Output results.
*/
stmt->tableElts = cxt.columns;
@@ -1915,6 +1922,34 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
}
/*
+ * transformCheckConstraints
+ * handle CHECK constraints
+ */
+static void
+transformCheckConstraints(CreateStmtContext *cxt, bool skipValidation)
+{
+ ListCell *ckclist;
+
+ if (cxt->ckconstraints == NIL)
+ return;
+ /*
+ * If CREATE TABLE we can safely skip validation of check
+ * constraints, and nonetheless mark them valid.
+ * (This will override any user-supplied NOT VALID flag.)
+ */
+ if (skipValidation)
+ {
+ foreach(ckclist, cxt->ckconstraints)
+ {
+ Constraint *constraint = (Constraint *) lfirst(ckclist);
+
+ constraint->skip_validation = true;
+ constraint->initially_valid = true;
+ }
+ }
+}
+
+/*
* transformFKConstraints
* handle FOREIGN KEY constraints
*/
Updated patch to add this table creation case in regression tests.
PFA patch version V3.
Regards,
Amul Sul
Attachments:
transformCheckConstraints-function-to-overrid-not-valid_V3.patchapplication/octet-streamDownload
From e8b76c375d7fe5ec739e3eee0382dcec2051e504 Mon Sep 17 00:00:00 2001
From: Amul Sul <sul_amul@yahoo.co.in>
Date: Wed, 16 Dec 2015 13:58:40 +0530
Subject: [PATCH] Make check constraint(s) valid at table creation even if NOT
VALID option provided
---
src/backend/catalog/heap.c | 2 +-
src/backend/parser/gram.y | 2 ++
src/backend/parser/parse_utilcmd.c | 35 ++++++++++++++++++++++++++++++
src/test/regress/expected/create_table.out | 18 +++++++++++++++
src/test/regress/sql/create_table.sql | 11 ++++++++++
5 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7d7d062..04c4f8f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2349,7 +2349,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
constrOid =
- StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
+ StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
is_local ? 0 : 1, cdef->is_no_inherit, is_internal);
numchecks++;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7916df8..c4bed8a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3080,6 +3080,8 @@ ColConstraintElem:
n->is_no_inherit = $5;
n->raw_expr = $3;
n->cooked_expr = NULL;
+ n->skip_validation = false;
+ n->initially_valid = true;
$$ = (Node *)n;
}
| DEFAULT b_expr
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 344a40c..566597b 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -120,6 +120,8 @@ static IndexStmt *transformIndexConstraint(Constraint *constraint,
static void transformFKConstraints(CreateStmtContext *cxt,
bool skipValidation,
bool isAddConstraint);
+static void transformCheckConstraints(CreateStmtContext *cxt,
+ bool skipValidation);
static void transformConstraintAttrs(CreateStmtContext *cxt,
List *constraintList);
static void transformColumnType(CreateStmtContext *cxt, ColumnDef *column);
@@ -320,6 +322,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
transformFKConstraints(&cxt, true, false);
/*
+ * Postprocess check constraints.
+ */
+ transformCheckConstraints(&cxt, true);
+
+ /*
* Output results.
*/
stmt->tableElts = cxt.columns;
@@ -1915,6 +1922,34 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
}
/*
+ * transformCheckConstraints
+ * handle CHECK constraints
+ */
+static void
+transformCheckConstraints(CreateStmtContext *cxt, bool skipValidation)
+{
+ ListCell *ckclist;
+
+ if (cxt->ckconstraints == NIL)
+ return;
+ /*
+ * If CREATE TABLE we can safely skip validation of check
+ * constraints, and nonetheless mark them valid.
+ * (This will override any user-supplied NOT VALID flag.)
+ */
+ if (skipValidation)
+ {
+ foreach(ckclist, cxt->ckconstraints)
+ {
+ Constraint *constraint = (Constraint *) lfirst(ckclist);
+
+ constraint->skip_validation = true;
+ constraint->initially_valid = true;
+ }
+ }
+}
+
+/*
* transformFKConstraints
* handle FOREIGN KEY constraints
*/
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 41ceb87..8a73970 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -253,3 +253,21 @@ DROP TABLE as_select1;
-- check that the oid column is added before the primary key is checked
CREATE TABLE oid_pk (f1 INT, PRIMARY KEY(oid)) WITH OIDS;
DROP TABLE oid_pk;
+-- at table creation, override NOT VALID flag
+CREATE TABLE foo (a SERIAL PRIMARY KEY);
+CREATE TABLE bar (a INT,
+ FOREIGN KEY(a) REFERENCES foo NOT VALID,
+ CHECK(a != 0) NOT VALID);
+\d bar
+ Table "public.bar"
+ Column | Type | Modifiers
+--------+---------+-----------
+ a | integer |
+Check constraints:
+ "bar_a_check" CHECK (a <> 0)
+Foreign-key constraints:
+ "bar_a_fkey" FOREIGN KEY (a) REFERENCES foo(a)
+
+--cleanup
+DROP TABLE bar;
+DROP TABLE foo;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 78bdc8b..3fb1675 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -269,3 +269,14 @@ DROP TABLE as_select1;
-- check that the oid column is added before the primary key is checked
CREATE TABLE oid_pk (f1 INT, PRIMARY KEY(oid)) WITH OIDS;
DROP TABLE oid_pk;
+
+-- at table creation, override NOT VALID flag
+CREATE TABLE foo (a SERIAL PRIMARY KEY);
+
+CREATE TABLE bar (a INT,
+ FOREIGN KEY(a) REFERENCES foo NOT VALID,
+ CHECK(a != 0) NOT VALID);
+\d bar
+--cleanup
+DROP TABLE bar;
+DROP TABLE foo;
--
2.6.2
On Wed, Dec 16, 2015 at 3:34 AM, amul sul <sul_amul@yahoo.co.in> wrote:
Updated patch to add this table creation case in regression tests.
PFA patch version V3.
I committed the previous version just now after fixing various things.
In particular, you added a function called from one place that took a
Boolean argument that always had the same value. You've got to either
call it from more than one place, or remove the argument. I picked
the former. Also, I added a test case and made a few other tweaks.
--
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