Fix comment in ATExecValidateConstraint

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers