Обсуждение: 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
Вложения
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
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], 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
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
Вложения
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
Вложения
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
Вложения
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