Re: using index or check in ALTER TABLE SET NOT NULL

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