Horton

Re: cataloguing NOT NULL constraints

Peter Eisentraut <peter@eisentraut.org>

View thread
From: Peter Eisentraut <peter@eisentraut.org>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>, pgsql-hackers@lists.postgresql.org
Date: 2023-07-03T14:02:37Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Revert structural changes to not-null constraints

  2. Fix inconsistencies in error messages

  3. Disallow direct change of NO INHERIT of not-null constraints

  4. Disallow NO INHERIT not-null constraints on partitioned tables

  5. Better handle indirect constraint drops

  6. Don't try to assign smart names to constraints

  7. Fix restore of not-null constraints with inheritance

  8. ATTACH PARTITION: Don't match a PK with a UNIQUE constraint

  9. Fix propagating attnotnull in multiple inheritance

  10. Check stack depth in new recursive functions

  11. Move privilege check to the right place

  12. Update information_schema definition for not-null constraints

  13. Fix not-null constraint test

  14. Disallow changing NO INHERIT status of a not-null constraint

  15. Catalog not-null constraints

  16. parallel_schedule: add comment on event_trigger test dependency

  17. Revert "Catalog NOT NULL constraints" and fallout

  18. Adjust contrib/sepgsql regression test expected outputs.

  19. Fix table name clash in recently introduced test

  20. Catalog NOT NULL constraints

  21. Change the rules for inherited CHECK constraints to be essentially the same

Attachments

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.