Thread: 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 :
http://www.postgresql.org/message-id/E1Q2Ak9-0006hK-M4@gemulon.postgresql.org)


IFAICU, initially_valid and skip_validation values are mutually exclusive at constraint creation(ref: gram.y), unless
itset 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
checkconstraint valid, when we have assurance that modified/added constraint are valid.
 

Comments? Thoughts? 

Regards,
Amul Sul

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-M4@gemulon.postgresql.org)
> 
> 
> IFAICU, initially_valid and skip_validation values are mutually exclusive at constraint creation(ref: gram.y), unless
itset 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
checkconstraint 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

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_validvalues, 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
Icould set both flag(initially_valid & skip_validation) to true.
 

Regards,
Amul Sul

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,
simplyI 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

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_validvalues, 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,
simplyI 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

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
bothpurpose, 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_validvalues, 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,
simplyI 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

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 :
http://www.postgresql.org/message-id/E1Q2Ak9-0006hK-M4@gemulon.postgresql.org)
>
>
> IFAICU, initially_valid and skip_validation values are mutually exclusive at constraint creation(ref: gram.y), unless
itset 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
justinstall 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

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

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

>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

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

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

>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

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

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

>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

Updated patch to add this table creation case in regression tests.
PFA patch version V3.


Regards,
Amul Sul

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