Re: [HACKERS] Fix comment in ATExecValidateConstraint

[Robert Haas]

On Thu, Aug 18, 2016 at 9:01 PM, Amit Langote
 wrote:
> Reads much less ambiguous, so +1.
>
> Except in the doc patch:
>
> s/change the type of a column constraint/change the type of a column/g
>
> I fixed that part in the attached.

Thanks.  Committed; sorry for the delay.

(As some of those of you following along at home may have noticed, I'm
catching up on old emails.)

-- 
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] Fix comment in ATExecValidateConstraint

[Amit Langote]

On 2016/08/19 5:35, Robert Haas wrote:
> On Thu, Aug 18, 2016 at 5:15 AM, Amit Langote
>  wrote:
>> On 2016/07/25 17:18, Amit Langote wrote:
>>> The comment seems to have been copied from ATExecAddColumn, which says:
>>>
>>>  /*
>>>   * If we are told not to recurse, there had better not be any
>>> - * child tables; else the addition would put them out of step.
>>>
>>> For ATExecValidateConstraint, it should say something like:
>>>
>>> + * child tables; else validating the constraint would put them
>>> + * out of step.
>>>
>>> Attached patch fixes it.
>>
>> I noticed that the ALTER TABLE documentation doesn't mention that VALIDATE
>> CONSTRAINT will fail if ONLY is specified and there are descendant tables.
>>  It only mentions that a constraint cannot be renamed unless also renamed
>> in the descendant tables.
>>
>> Attached patch fixes that.
> 
> I did some wordsmithing on the two patches you posted to this thread.
> I suggest the attached version.  What do you think?

Reads much less ambiguous, so +1.

Except in the doc patch:

s/change the type of a column constraint/change the type of a column/g

I fixed that part in the attached.

Thanks,
Amit
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6f51cbc..e48ccf2 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1028,11 +1028,15 @@ ALTER TABLE ALL IN TABLESPACE name
 

 If a table has any descendant tables, it is not permitted to add,
-rename, or change the type of a column, or rename an inherited constraint
-in the parent table without doing
-the same to the descendants.  That is, ALTER TABLE ONLY
-will be rejected.  This ensures that the descendants always have
-columns matching the parent.
+rename, or change the type of a column in the parent table without doing
+same to the descendants.  This ensures that the descendants always have
+columns matching the parent.  Similarly, a constraint cannot be renamed
+in the parent without also renaming it in all descendents, so that
+constraints also match between the parent and its descendents.
+Also, because selecting from the parent also selects from its descendents,
+a constraint on the parent cannot be marked valid unless it is also marked
+valid for those descendents.  In all of these cases, ALTER TABLE
+ONLY will be rejected.  

 

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..d312762 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6908,7 +6908,8 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
 
 /*
  * If we are told not to recurse, there had better not be any
- * child tables; else the addition would put them out of step.
+ * child tables, because we can't mark the constraint on the
+ * parent valid unless it is valid for all child tables.
  */
 if (!recurse)
 	ereport(ERROR,

-- 
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] Fix comment in ATExecValidateConstraint

[Robert Haas]

On Thu, Aug 18, 2016 at 5:15 AM, Amit Langote
 wrote:
> On 2016/07/25 17:18, Amit Langote wrote:
>> The comment seems to have been copied from ATExecAddColumn, which says:
>>
>>  /*
>>   * If we are told not to recurse, there had better not be any
>> - * child tables; else the addition would put them out of step.
>>
>> For ATExecValidateConstraint, it should say something like:
>>
>> + * child tables; else validating the constraint would put them
>> + * out of step.
>>
>> Attached patch fixes it.
>
> I noticed that the ALTER TABLE documentation doesn't mention that VALIDATE
> CONSTRAINT will fail if ONLY is specified and there are descendant tables.
>  It only mentions that a constraint cannot be renamed unless also renamed
> in the descendant tables.
>
> Attached patch fixes that.

I did some wordsmithing on the two patches you posted to this thread.
I suggest the attached version.  What do you think?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6f51cbc..a070041 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1028,11 +1028,15 @@ ALTER TABLE ALL IN TABLESPACE name
 

 If a table has any descendant tables, it is not permitted to add,
-rename, or change the type of a column, or rename an inherited constraint
-in the parent table without doing
-the same to the descendants.  That is, ALTER TABLE ONLY
-will be rejected.  This ensures that the descendants always have
-columns matching the parent.
+rename, or change the type of a column constraint in the parent table
+without doing same to the descendants.  This ensures that the descendants
+always have columns matching the parent.  Similarly, a constraint cannot be
+renamed in the parent without also renaming it in all descendents, so that
+constraints also match between the parent and its descendents.
+Also, because selecting from the parent also selects from its descendents,
+a constraint on the parent cannot be marked valid unless it is also marked
+valid for those descendents.  In all of these cases, ALTER TABLE
+ONLY will be rejected.  

 

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..d312762 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6908,7 +6908,8 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
 
 /*
  * If we are told not to recurse, there had better not be any
- * child tables; else the addition would put them out of step.
+ * child tables, because we can't mark the constraint on the
+ * parent valid unless it is valid for all child tables.
  */
 if (!recurse)
 	ereport(ERROR,

-- 
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] Fix comment in ATExecValidateConstraint

[Amit Langote]

On 2016/07/25 17:18, Amit Langote wrote:
> The comment seems to have been copied from ATExecAddColumn, which says:
> 
>  /*
>   * If we are told not to recurse, there had better not be any
> - * child tables; else the addition would put them out of step.
> 
> For ATExecValidateConstraint, it should say something like:
> 
> + * child tables; else validating the constraint would put them
> + * out of step.
> 
> Attached patch fixes it.

I noticed that the ALTER TABLE documentation doesn't mention that VALIDATE
CONSTRAINT will fail if ONLY is specified and there are descendant tables.
 It only mentions that a constraint cannot be renamed unless also renamed
in the descendant tables.

Attached patch fixes that.

Thanks,
Amit
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 6f51cbc..975b395 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1028,11 +1028,11 @@ ALTER TABLE ALL IN TABLESPACE name
 

 If a table has any descendant tables, it is not permitted to add,
-rename, or change the type of a column, or rename an inherited constraint
-in the parent table without doing
+rename, or change the type of a column, rename or validate an inherited
+constraint in the parent table without doing
 the same to the descendants.  That is, ALTER TABLE ONLY
 will be rejected.  This ensures that the descendants always have
-columns matching the parent.
+columns and constraints matching the parent.

 


-- 
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] Fix comment in ATExecValidateConstraint

[Amit Langote]

On 2016/07/29 23:50, Robert Haas wrote:
> On Mon, Jul 25, 2016 at 4:18 AM, Amit Langote
>  wrote:
>> The comment seems to have been copied from ATExecAddColumn, which says:
>>
>>  /*
>>   * If we are told not to recurse, there had better not be any
>> - * child tables; else the addition would put them out of step.
>>
>> For ATExecValidateConstraint, it should say something like:
>>
>> + * child tables; else validating the constraint would put them
>> + * out of step.
>>
>> Attached patch fixes it.
> 
> I agree that the current comment is wrong, but what does "out of step"
> actually mean here, anyway?  I think this isn't very clear.

Like Tom explained over at [1], I guess it means if a constraint is marked
validated in parent, the same constraint in all the children should have
been marked validated as well. Validating just the parent's copy seems to
break this invariant. I admit though that I don't know if the phrase "out
of step" conveys that.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/13658.1470072749%40sss.pgh.pa.us




-- 
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] Fix comment in ATExecValidateConstraint

[Robert Haas]

On Mon, Jul 25, 2016 at 4:18 AM, Amit Langote
 wrote:
> The comment seems to have been copied from ATExecAddColumn, which says:
>
>  /*
>   * If we are told not to recurse, there had better not be any
> - * child tables; else the addition would put them out of step.
>
> For ATExecValidateConstraint, it should say something like:
>
> + * child tables; else validating the constraint would put them
> + * out of step.
>
> Attached patch fixes it.

I agree that the current comment is wrong, but what does "out of step"
actually mean here, anyway?  I think this isn't very clear.

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