Re: cataloguing NOT NULL constraints

From: Peter Eisentraut <peter(at)eisentraut(dot)org>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: cataloguing NOT NULL constraints
Date: 2023-07-03 14:02:37
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 ==

* 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
+ * created. Also, duplicate NOT NULL declarations are not
+ * allowed.
+ */

About this:

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

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
+ * 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 =
+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.

Attachment Content-Type Size
0001-fixup-Add-pg_constraint-rows-for-NOT-NULL-constraint.patch.nocfbot text/plain 4.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2023-07-03 14:07:53 Re: Mark a transaction uncommittable
Previous Message Daniel Gustafsson 2023-07-03 13:57:50 Re: Introduce "log_connection_stages" setting.