Re: [HACKERS] Fix comment in ATExecValidateConstraint
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
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
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
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
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
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