Re: Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints

hi.

the following are reviews of changes in pg_dump
on v6-0001-NOT-NULL-NOT-VALID.patch

minor style tweak:
+ "CASE WHEN NOT co.convalidated THEN co.oid"
+ " ELSE NULL END AS notnull_invalidoid,\n"

align with surrounding code convention:
leave white space at the end, not beginning.
maybe we can
+ "CASE WHEN NOT co.convalidated THEN co.oid "
+ "ELSE NULL END AS notnull_invalidoid,\n"

+ * For invalid constraints, we need to store their OIDs for processing
+ * elsewhere, so we bring the pg_constraint.oid value when the constraint
+ * is invalid, and NULL otherwise.
+ *
looking at below surrounding code
if (fout->remoteVersion >= 180000)
appendPQExpBufferStr(q,
" LEFT JOIN pg_catalog.pg_constraint co ON "
"(a.attrelid = co.conrelid\n"
" AND co.contype = 'n' AND "
"co.conkey = array[a.attnum])\n");
so this will only apply to the not-null constraint currently.
maybe we should say:
+ * For invalid not-null constraints, we need to store their OIDs for processing

I have some confusion about determineNotNullFlags comments.
* 3) The column has an invalid not-null constraint. This must be treated
* as a separate object (because it must be created after the table data
* is loaded). So we add its OID to invalidnotnulloids for processing
* elsewhere and do nothing further with it here. We distinguish this
* case because the "invalidoid" column has been set to a non-NULL value,
* which is the constraint OID. Valid constraints have a null OID.

The last sentence is not clear to me. Maybe I failed to grasp the
English language implicit
reference. i think it should be:

* We distinguish this
* case because the "notnull_invalidoid" column has been set to a
non-NULL value,
* which is the constraint OID. for valid not-null constraints, this
column is NULL value.

determineNotNullFlags comments:
* In case 3 above, the name comparison is a bit of a hack;
should change to
* In case 4 above, the name comparison is a bit of a hack;
?

--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -365,10 +365,11 @@ typedef struct _tableInfo
* there isn't one on this column. If
* empty string, unnamed constraint
* (pre-v17) */
+ bool *notnull_validated; /* true if NOT NULL is valid */
notnull_validated was never being used, should we remove it?

minor style tweak:
+ "CASE WHEN NOT co.convalidated THEN co.oid"
+ " ELSE NULL END AS notnull_invalidoid,\n"

align with surrounding code convention:
leave white space at the end, not beginning.
maybe we can
+ "CASE WHEN NOT co.convalidated THEN co.oid "
+ "ELSE NULL END AS notnull_invalidoid,\n"

+ * For invalid constraints, we need to store their OIDs for processing
+ * elsewhere, so we bring the pg_constraint.oid value when the constraint
+ * is invalid, and NULL otherwise.
+ *
looking at below surrounding code
if (fout->remoteVersion >= 180000)
appendPQExpBufferStr(q,
" LEFT JOIN pg_catalog.pg_constraint co ON "
"(a.attrelid = co.conrelid\n"
" AND co.contype = 'n' AND "
"co.conkey = array[a.attnum])\n");
so this will only apply to the not-null constraint currently.
maybe we should say:
+ * For invalid not-null constraints, we need to store their OIDs for processing

I have some confusion about determineNotNullFlags comments.
* 3) The column has an invalid not-null constraint. This must be treated
* as a separate object (because it must be created after the table data
* is loaded). So we add its OID to invalidnotnulloids for processing
* elsewhere and do nothing further with it here. We distinguish this
* case because the "invalidoid" column has been set to a non-NULL value,
* which is the constraint OID. Valid constraints have a null OID.

The last sentence is not clear to me. Maybe I failed to grasp the
English language implicit
reference. i think it should be:

* We distinguish this
* case because the "notnull_invalidoid" column has been set to a
non-NULL value,
* which is the constraint OID. for valid not-null constraints, this
column is NULL value.

determineNotNullFlags comments:
* In case 3 above, the name comparison is a bit of a hack;
should change to
* In case 4 above, the name comparison is a bit of a hack;
?

--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -365,10 +365,11 @@ typedef struct _tableInfo
* there isn't one on this column. If
* empty string, unnamed constraint
* (pre-v17) */
+ bool *notnull_validated; /* true if NOT NULL is valid */
notnull_validated was never being used, should we remove it?