From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
Cc: | Sergei Kornilov <sk(at)zsrv(dot)org>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Ildar Musin <i(dot)musin(at)postgrespro(dot)ru>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: using index or check in ALTER TABLE SET NOT NULL |
Date: | 2019-03-12 14:44:47 |
Message-ID: | CA+TgmobvUHVkOVpMkNU=BA5nCyn4GkQ3dCVzj0qdCBX8af_q=Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sun, Mar 10, 2019 at 11:30 PM David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> Looks good to me. Good idea to keep the controversial setting of
> client_min_messages to debug1 in the tests in a separate patch.
>
> Apart from a few lines that need to be wrapped at 80 chars and some
> comment lines that seem to have been wrapped early, I'm happy for it
> to be marked as ready for committer, but I'll defer to Ildar in case
> he wants to have another look.
Dispatches from the department of grammatical nitpicking...
+ entire table, however if a valid <literal>CHECK</literal> constraint is
I think this should be:
entire table; however, if...
+ * are set NOT NULL, however, if we can find a constraint which proves
similarly here
+ ereport(DEBUG1,
+ (errmsg("verifying table \"%s\" NOT NULL constraint "
+ "on %s attribute by existed constraints",
+ RelationGetRelationName(rel), NameStr(attr->attname))));
Ugh, that doesn't read well at all. How about:
existing constraints on column "%s"."%s" are sufficient to prove that
it does not contain nulls
- * in implicit-AND form.
+ * in implicitly-AND form, must only contain immutable clauses
+ * and all Vars must be varno=1.
I think you should leave the existing sentence alone (implicit-AND is
correct, implicitly-AND is not) and add a new sentence that says the
stuff you want to add.
+ * "Existing constraints" include its check constraints and optional
+ * caller-provided existConstraint list. existConstraint list is modified
+ * during ConstraintImpliedByRelConstraint call and would represent all
+ * assumed conditions. testConstraint describes the constraint to validate.
+ * Both existConstraint and testConstraint must be in implicitly-AND form,
+ * must only contain immutable clauses and all Vars must be varno=1.
I think that it might be better to copy the list rather than to have
the comment note that it gets mutated, but regardless the grammar
needs improvement here. Maybe: On entry, existConstraint is a
caller-provided list of conditions which this function may assume to
be true; on exit, it will have been destructively modified by the
addition of the table's CHECK constraints. testConstraint is the
constraint to validate. Both existConstraint and testConstraint must
be in implicit-AND form, must only contain immutable clauses, and must
contain only Vars with varno = 1.
- * not-false and try to prove the same for partConstraint.
- *
- * Note that predicate_implied_by assumes its first argument is known
- * immutable. That should always be true for partition constraints, so we
- * don't test it here.
+ * not-false and try to prove the same for testConstraint.
I object to removing this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Evgeniy Efimkin | 2019-03-12 14:54:30 | Re: Special role for subscriptions |
Previous Message | Paul Ramsey | 2019-03-12 14:40:43 | Re: Compressed TOAST Slicing |