Re: Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints - Mailing list pgsql-hackers

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?