Re: cataloguing NOT NULL constraints

On 30.06.23 13:44, Alvaro Herrera wrote:
> OK, so here's a new attempt to get this working correctly.

Attached is a small fixup patch for the documentation.

Furthermore, there are a few outdated comments that are probably left
over from previous versions of this patch set.

* src/backend/catalog/pg_constraint.c

Outdated comment:

+ /* only tuples for CHECK constraints should be given */
+ Assert(((Form_pg_constraint) GETSTRUCT(constrTup))->contype ==
CONSTRAINT_NOTNULL);

* src/backend/parser/gram.y

Should processCASbits() set &n->skip_validation, like in the CHECK
case? _outConstraint() looks at the field, so it seems relevant.

* src/backend/parser/parse_utilcmd.c

The updated comment says

List *ckconstraints; /* CHECK and NOT NULL constraints */

but it seems to me that NOT NULL constraints are not actually added
there but in nnconstraints instead.

It would be nice if the field nnconstraints was listed after
ckconstraints consistently throughout the file. It's a bit random
right now.

This comment is outdated:

+ /*
+ * For NOT NULL declarations, we need to mark the column as
+ * not nullable, and set things up to have a CHECK
constraint
+ * created. Also, duplicate NOT NULL declarations are not
+ * allowed.
+ */

About this:

case CONSTR_CHECK:
cxt->ckconstraints = lappend(cxt->ckconstraints,
constraint);
+
+ /*
+ * XXX If the user says CHECK (IS NOT NULL), should we turn
+ * that into a regular NOT NULL constraint?
+ */
break;

I think this was decided against.

+ /*
+ * Copy NOT NULL constraints, too (these do not require any option
to have
+ * been given).
+ */

Shouldn't that be governed by the INCLUDING CONSTRAINTS option?

Btw., there is some asymmetry here between check constraints and
not-null constraints: Check constraints are in the tuple descriptor,
but not-null constraints are not. Should that be unified? Or at
least explained?

* src/include/nodes/parsenodes.h

This comment appears to be outdated:

+ * intermixed in tableElts, and constraints and notnullcols are NIL. After
+ * parse analysis, tableElts contains just ColumnDefs, notnullcols has been
+ * filled with not-nullable column names from various sources, and
constraints
+ * contains just Constraint nodes (in fact, only CONSTR_CHECK nodes, in the
+ * present implementation).

There is no "notnullcolls".

* src/test/regress/parallel_schedule

This change appears to be correct, but unrelated to this patch, so I
suggest committing this separately.

* src/test/regress/sql/create_table.sql

-SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass;
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass ORDER BY coninhcount DESC, conname;

Maybe add conname to the select list here as well, for consistency with
nearby queries.