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

Hello, thanks for the review.

On 2025-Apr-02, jian he wrote:

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

I put that space there on purpose, actually, because it makes it clear
that the second line is subsidiary to the previous one (a continuation
of the CASE expression).  But you're right, we don't do this elsewhere
in the same query, so I'll use the style you suggest.

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

Well, the whole comment is talking only about not-null constraints, so
it didn't seem necessary.

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

I agree with the change of "invalidoid" to "notnull_invalidoid", because
that's the actual name of the column.  But I don't think we need the
second change (from "Valid constraints have..." to "For valid not-null
constraints ...") because the whole function is only concerned with
not-null constraints, and the whole comment is only talking about
not-null constraints, so I think it's quite clear that the constraints
in question are not-null constraints.


> 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;
> ?

Ah yes, thanks.


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

You're right, thanks.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"This is what I like so much about PostgreSQL.  Most of the surprises
are of the "oh wow!  That's cool" Not the "oh shit!" kind.  :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php