Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-16 Thread Robert Haas
On Wed, Dec 16, 2015 at 3:34 AM, amul sul  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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-16 Thread amul sul
Updated patch to add this table creation case in regression tests.
PFA patch version V3.

 
Regards,
Amul Sul

transformCheckConstraints-function-to-overrid-not-valid_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] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread amul sul
>On Wednesday, 9 December 2015 12:55 PM, Amit Langote 
> 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

transformCheckConstraints-function-to-overrid-not-valid.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] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread Amit Langote
On Wednesday, 9 December 2015, amul sul  wrote:

> >On Wednesday, 9 December 2015 12:55 PM, Amit Langote <
> langote_amit...@lab.ntt.co.jp > 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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread Amit Langote
On 2015/12/09 18:07, amul sul wrote:
>> On Wednesday, 9 December 2015 12:55 PM, Amit Langote 
>>  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
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 */

-- 
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] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread Amit Langote
On 2015/12/10 13:12, amul sul wrote:
>> On Thursday, 10 December 2015 8:22 AM, Amit Langote 
>>  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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread amul sul
>On Thursday, 10 December 2015 10:13 AM, Amit Langote 
> 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

transformCheckConstraints-function-to-overrid-not-valid_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] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread Amit Langote
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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-09 Thread amul sul
>On Thursday, 10 December 2015 8:22 AM, Amit Langote 
> 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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-08 Thread Robert Haas
On Thu, Dec 3, 2015 at 3:52 AM, amul sul  wrote:
> Hi ALL,
>
> Need your suggestions.
> initially_valid flag is added to make column constraint valid. (commit : 
> http://www.postgresql.org/message-id/e1q2ak9-0006hk...@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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-08 Thread Amit Langote
On 2015/12/09 5:50, Robert Haas wrote:
> On Thu, Dec 3, 2015 at 3:52 AM, amul sul  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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-08 Thread Amit Langote
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
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:
-

Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-03 Thread amul sul
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 
 wrote:
On 2015/12/03 20:44, amul sul wrote:
>> On Thursday, 3 December 2015 4:36 PM, Amit Langote 
>>  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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-03 Thread amul sul
Hi Amit,

Thanks for prompt response.

>On Thursday, 3 December 2015 4:36 PM, Amit Langote 
> 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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-03 Thread Marko Tiikkaja

On 12/3/15 12:44 PM, amul sul wrote:

On Thursday, 3 December 2015 4:36 PM, Amit Langote 
 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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-03 Thread Amit Langote

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 : 
> http://www.postgresql.org/message-id/e1q2ak9-0006hk...@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


Re: [HACKERS] Passing initially_valid values instead of !skip_validation to StoreRelCheck() in AddRelationNewConstraints()

2015-12-03 Thread Amit Langote
On 2015/12/03 20:44, amul sul wrote:
>> On Thursday, 3 December 2015 4:36 PM, Amit Langote 
>>  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