Fix comment in ATExecValidateConstraint
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.
Thanks,
Amit
Attachments:
update-comment.patchtext/x-diff; name=update-comment.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..ddb2f2a 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; else validating the constraint would put them
+ * out of step.
*/
if (!recurse)
ereport(ERROR,
On Mon, Jul 25, 2016 at 4:18 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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
On 2016/07/29 23:50, Robert Haas wrote:
On Mon, Jul 25, 2016 at 4:18 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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]/messages/by-id/13658.1470072749@sss.pgh.pa.us, 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]: /messages/by-id/13658.1470072749@sss.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
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
Attachments:
alter-table-only-doc.patchtext/x-diff; name=alter-table-only-doc.patchDownload
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 <replaceable class="PARAMETER">name</replaceable>
<para>
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, <command>ALTER TABLE ONLY</command>
will be rejected. This ensures that the descendants always have
- columns matching the parent.
+ columns and constraints matching the parent.
</para>
<para>
On Thu, Aug 18, 2016 at 5:15 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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
Attachments:
inheritance-wordsmithing-revised.patchtext/x-diff; charset=US-ASCII; name=inheritance-wordsmithing-revised.patchDownload
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 <replaceable class="PARAMETER">name</replaceable>
<para>
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, <command>ALTER TABLE ONLY</command>
- 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, <command>ALTER TABLE
+ ONLY</command> will be rejected.
</para>
<para>
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,
On 2016/08/19 5:35, Robert Haas wrote:
On Thu, Aug 18, 2016 at 5:15 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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
Attachments:
inheritance-wordsmithing-revised-2.patchtext/x-diff; name=inheritance-wordsmithing-revised-2.patchDownload
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 <replaceable class="PARAMETER">name</replaceable>
<para>
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, <command>ALTER TABLE ONLY</command>
- 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, <command>ALTER TABLE
+ ONLY</command> will be rejected.
</para>
<para>
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,
On Thu, Aug 18, 2016 at 9:01 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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