Feedback-ID: ie0a040ee:Fastmail
Content-Type: multipart/mixed; boundary="------------q7KBgnbaAceYB0bjXwYrr702"
Message-ID: <176949aa-74dc-535b-c9be-930a81853aae@eisentraut.org>
Date: Mon, 3 Jul 2023 16:02:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: cataloguing NOT NULL constraints
Content-Language: en-US
To: Alvaro Herrera <alvherre@alvh.no-ip.org>,
 pgsql-hackers@lists.postgresql.org
References: <20230630114403.nru7f6i5ejgcxz42@alvherre.pgsql>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <20230630114403.nru7f6i5ejgcxz42@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/176949aa-74dc-535b-c9be-930a81853aae%40eisentraut.org>
Precedence: bulk

This is a multi-part message in MIME format.
--------------q7KBgnbaAceYB0bjXwYrr702
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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.

--------------q7KBgnbaAceYB0bjXwYrr702
Content-Type: text/plain; charset=UTF-8;
 name="0001-fixup-Add-pg_constraint-rows-for-NOT-NULL-constraint.patch.nocfbot"
Content-Disposition: attachment;
 filename*0="0001-fixup-Add-pg_constraint-rows-for-NOT-NULL-constraint.pa";
 filename*1="tch.nocfbot"
Content-Transfer-Encoding: base64

RnJvbSBjNDgxYjU4YzNlNDhmZjBhYjQ3MzhlNWNmMjQ0MGQyYWQ2ZmMzZTQ3IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogTW9uLCAzIEp1bCAyMDIzIDE1OjQ0OjU5ICswMjAwClN1YmplY3Q6
IFtQQVRDSF0gZml4dXAhIEFkZCBwZ19jb25zdHJhaW50IHJvd3MgZm9yIE5PVCBOVUxMIGNv
bnN0cmFpbnRzCgotLS0KIGRvYy9zcmMvc2dtbC9jYXRhbG9ncy5zZ21sICAgICAgICB8IDIg
Ky0KIGRvYy9zcmMvc2dtbC9yZWYvYWx0ZXJfdGFibGUuc2dtbCB8IDkgKysrKy0tLS0tCiAy
IGZpbGVzIGNoYW5nZWQsIDUgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkKCmRpZmYg
LS1naXQgYS9kb2Mvc3JjL3NnbWwvY2F0YWxvZ3Muc2dtbCBiL2RvYy9zcmMvc2dtbC9jYXRh
bG9ncy5zZ21sCmluZGV4IDkxMzdiMWJjNTguLjM2NDYwMTI2ZjQgMTAwNjQ0Ci0tLSBhL2Rv
Yy9zcmMvc2dtbC9jYXRhbG9ncy5zZ21sCisrKyBiL2RvYy9zcmMvc2dtbC9jYXRhbG9ncy5z
Z21sCkBAIC0yNTUyLDcgKzI1NTIsNyBAQCA8dGl0bGU+PHN0cnVjdG5hbWU+cGdfY29uc3Ry
YWludDwvc3RydWN0bmFtZT4gQ29sdW1uczwvdGl0bGU+CiAgICAgICA8cGFyYT4KICAgICAg
ICA8bGl0ZXJhbD5jPC9saXRlcmFsPiA9IGNoZWNrIGNvbnN0cmFpbnQsCiAgICAgICAgPGxp
dGVyYWw+ZjwvbGl0ZXJhbD4gPSBmb3JlaWduIGtleSBjb25zdHJhaW50LAotICAgICAgIDxs
aXRlcmFsPm48L2xpdGVyYWw+ID0gbm90IG51bGwgY29uc3RyYWludCwKKyAgICAgICA8bGl0
ZXJhbD5uPC9saXRlcmFsPiA9IG5vdC1udWxsIGNvbnN0cmFpbnQsCiAgICAgICAgPGxpdGVy
YWw+cDwvbGl0ZXJhbD4gPSBwcmltYXJ5IGtleSBjb25zdHJhaW50LAogICAgICAgIDxsaXRl
cmFsPnU8L2xpdGVyYWw+ID0gdW5pcXVlIGNvbnN0cmFpbnQsCiAgICAgICAgPGxpdGVyYWw+
dDwvbGl0ZXJhbD4gPSBjb25zdHJhaW50IHRyaWdnZXIsCmRpZmYgLS1naXQgYS9kb2Mvc3Jj
L3NnbWwvcmVmL2FsdGVyX3RhYmxlLnNnbWwgYi9kb2Mvc3JjL3NnbWwvcmVmL2FsdGVyX3Rh
YmxlLnNnbWwKaW5kZXggMGI2NTczMWIxZi4uMmM0MTM4ZTRlOSAxMDA2NDQKLS0tIGEvZG9j
L3NyYy9zZ21sL3JlZi9hbHRlcl90YWJsZS5zZ21sCisrKyBiL2RvYy9zcmMvc2dtbC9yZWYv
YWx0ZXJfdGFibGUuc2dtbApAQCAtMTEzLDEzICsxMTMsMTIgQEAKIAogWyBDT05TVFJBSU5U
IDxyZXBsYWNlYWJsZSBjbGFzcz0icGFyYW1ldGVyIj5jb25zdHJhaW50X25hbWU8L3JlcGxh
Y2VhYmxlPiBdCiB7IENIRUNLICggPHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPmV4
cHJlc3Npb248L3JlcGxhY2VhYmxlPiApIFsgTk8gSU5IRVJJVCBdIHwKKyAgTk9UIE5VTEwg
PHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPmNvbHVtbl9uYW1lPC9yZXBsYWNlYWJs
ZT4gWyBOTyBJTkhFUklUIF0gfAogICBVTklRVUUgWyBOVUxMUyBbIE5PVCBdIERJU1RJTkNU
IF0gKCA8cmVwbGFjZWFibGUgY2xhc3M9InBhcmFtZXRlciI+Y29sdW1uX25hbWU8L3JlcGxh
Y2VhYmxlPiBbLCAuLi4gXSApIDxyZXBsYWNlYWJsZSBjbGFzcz0icGFyYW1ldGVyIj5pbmRl
eF9wYXJhbWV0ZXJzPC9yZXBsYWNlYWJsZT4gfAogICBQUklNQVJZIEtFWSAoIDxyZXBsYWNl
YWJsZSBjbGFzcz0icGFyYW1ldGVyIj5jb2x1bW5fbmFtZTwvcmVwbGFjZWFibGU+IFssIC4u
LiBdICkgPHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPmluZGV4X3BhcmFtZXRlcnM8
L3JlcGxhY2VhYmxlPiB8CiAgIEVYQ0xVREUgWyBVU0lORyA8cmVwbGFjZWFibGUgY2xhc3M9
InBhcmFtZXRlciI+aW5kZXhfbWV0aG9kPC9yZXBsYWNlYWJsZT4gXSAoIDxyZXBsYWNlYWJs
ZSBjbGFzcz0icGFyYW1ldGVyIj5leGNsdWRlX2VsZW1lbnQ8L3JlcGxhY2VhYmxlPiBXSVRI
IDxyZXBsYWNlYWJsZSBjbGFzcz0icGFyYW1ldGVyIj5vcGVyYXRvcjwvcmVwbGFjZWFibGU+
IFssIC4uLiBdICkgPHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPmluZGV4X3BhcmFt
ZXRlcnM8L3JlcGxhY2VhYmxlPiBbIFdIRVJFICggPHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJh
bWV0ZXIiPnByZWRpY2F0ZTwvcmVwbGFjZWFibGU+ICkgXSB8CiAgIEZPUkVJR04gS0VZICgg
PHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPmNvbHVtbl9uYW1lPC9yZXBsYWNlYWJs
ZT4gWywgLi4uIF0gKSBSRUZFUkVOQ0VTIDxyZXBsYWNlYWJsZSBjbGFzcz0icGFyYW1ldGVy
Ij5yZWZ0YWJsZTwvcmVwbGFjZWFibGU+IFsgKCA8cmVwbGFjZWFibGUgY2xhc3M9InBhcmFt
ZXRlciI+cmVmY29sdW1uPC9yZXBsYWNlYWJsZT4gWywgLi4uIF0gKSBdCi0gICAgWyBNQVRD
SCBGVUxMIHwgTUFUQ0ggUEFSVElBTCB8IE1BVENIIFNJTVBMRSBdIFsgT04gREVMRVRFIDxy
ZXBsYWNlYWJsZSBjbGFzcz0icGFyYW1ldGVyIj5yZWZlcmVudGlhbF9hY3Rpb248L3JlcGxh
Y2VhYmxlPiBdIFsgT04gVVBEQVRFIDxyZXBsYWNlYWJsZSBjbGFzcz0icGFyYW1ldGVyIj5y
ZWZlcmVudGlhbF9hY3Rpb248L3JlcGxhY2VhYmxlPiBdIHwKLSAgTk9UIE5VTEwgPHJlcGxh
Y2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPmNvbHVtbl9uYW1lPC9yZXBsYWNlYWJsZT4gWyBO
TyBJTkhFUklUIF0KLX0KKyAgICBbIE1BVENIIEZVTEwgfCBNQVRDSCBQQVJUSUFMIHwgTUFU
Q0ggU0lNUExFIF0gWyBPTiBERUxFVEUgPHJlcGxhY2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIi
PnJlZmVyZW50aWFsX2FjdGlvbjwvcmVwbGFjZWFibGU+IF0gWyBPTiBVUERBVEUgPHJlcGxh
Y2VhYmxlIGNsYXNzPSJwYXJhbWV0ZXIiPnJlZmVyZW50aWFsX2FjdGlvbjwvcmVwbGFjZWFi
bGU+IF0gfQogWyBERUZFUlJBQkxFIHwgTk9UIERFRkVSUkFCTEUgXSBbIElOSVRJQUxMWSBE
RUZFUlJFRCB8IElOSVRJQUxMWSBJTU1FRElBVEUgXQogCiA8cGhyYXNlPmFuZCA8cmVwbGFj
ZWFibGUgY2xhc3M9InBhcmFtZXRlciI+dGFibGVfY29uc3RyYWludF91c2luZ19pbmRleDwv
cmVwbGFjZWFibGU+IGlzOjwvcGhyYXNlPgpAQCAtMTc2NSw3ICsxNzY0LDcgQEAgPHRpdGxl
PkV4YW1wbGVzPC90aXRsZT4KICAgPHRpdGxlPkNvbXBhdGliaWxpdHk8L3RpdGxlPgogCiAg
IDxwYXJhPgotICAgVGhlIGZvcm1zIDxsaXRlcmFsPkFERCBDT0xVTU48L2xpdGVyYWw+Cisg
ICBUaGUgZm9ybXMgPGxpdGVyYWw+QUREIFtDT0xVTU5dPC9saXRlcmFsPiwKICAgIDxsaXRl
cmFsPkRST1AgW0NPTFVNTl08L2xpdGVyYWw+LCA8bGl0ZXJhbD5EUk9QIElERU5USVRZPC9s
aXRlcmFsPiwgPGxpdGVyYWw+UkVTVEFSVDwvbGl0ZXJhbD4sCiAgICA8bGl0ZXJhbD5TRVQg
REVGQVVMVDwvbGl0ZXJhbD4sIDxsaXRlcmFsPlNFVCBEQVRBIFRZUEU8L2xpdGVyYWw+ICh3
aXRob3V0IDxsaXRlcmFsPlVTSU5HPC9saXRlcmFsPiksCiAgICA8bGl0ZXJhbD5TRVQgR0VO
RVJBVEVEPC9saXRlcmFsPiwgYW5kIDxsaXRlcmFsPlNFVCA8cmVwbGFjZWFibGU+c2VxdWVu
Y2Vfb3B0aW9uPC9yZXBsYWNlYWJsZT48L2xpdGVyYWw+CkBAIC0xNzczLDcgKzE3NzIsNyBA
QCA8dGl0bGU+Q29tcGF0aWJpbGl0eTwvdGl0bGU+CiAgICBUaGUgZm9ybSA8bGl0ZXJhbD5B
REQgPHJlcGxhY2VhYmxlPnRhYmxlX2NvbnN0cmFpbnQ8L3JlcGxhY2VhYmxlPjwvbGl0ZXJh
bD4KICAgIGNvbmZvcm1zIHdpdGggdGhlIFNRTCBzdGFuZGFyZCB3aGVuIHRoZSA8bGl0ZXJh
bD5VU0lORyBJTkRFWDwvbGl0ZXJhbD4gYW5kCiAgICA8bGl0ZXJhbD5OT1QgVkFMSUQ8L2xp
dGVyYWw+IGNsYXVzZXMgYXJlIG9taXR0ZWQgYW5kIHRoZSBjb25zdHJhaW50IHR5cGUgaXMK
LSAgIG9uZSBvZiA8bGl0ZXJhbD5VTklRVUU8L2xpdGVyYWw+LCA8bGl0ZXJhbD5QUklNQVJZ
IEtFWTwvbGl0ZXJhbD4KKyAgIG9uZSBvZiA8bGl0ZXJhbD5DSEVDSzwvbGl0ZXJhbD4sIDxs
aXRlcmFsPlVOSVFVRTwvbGl0ZXJhbD4sIDxsaXRlcmFsPlBSSU1BUlkgS0VZPC9saXRlcmFs
PiwKICAgIG9yIDxsaXRlcmFsPlJFRkVSRU5DRVM8L2xpdGVyYWw+LgogICAgVGhlIG90aGVy
IGZvcm1zIGFyZQogICAgPHByb2R1Y3RuYW1lPlBvc3RncmVTUUw8L3Byb2R1Y3RuYW1lPiBl
eHRlbnNpb25zIG9mIHRoZSBTUUwgc3RhbmRhcmQuCi0tIAoyLjQxLjAKCg==

--------------q7KBgnbaAceYB0bjXwYrr702--