Feedback-ID: ia2694551:Fastmail
Date: Tue, 24 Sep 2024 11:43:26 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: not null constraints, again
Message-ID: <202409240943.vybpb5nq7vt3@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxHCLJUNCq0jtKCo+6UNC6Qdfm-0gof8MU=RmP25cLNOyQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202409240943.vybpb5nq7vt3%40alvherre.pgsql>
Precedence: bulk

On 2024-Sep-24, jian he wrote:

> static Oid
> StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum,
>                 bool is_validated, bool is_local, int inhcount,
>                 bool is_no_inherit)
> {
>     Oid            constrOid;
>     Assert(attnum > InvalidAttrNumber);
>     constrOid =
>         CreateConstraintEntry(nnname,
>                               RelationGetNamespace(rel),
>                               CONSTRAINT_NOTNULL,
>                               false,
>                               false,
>                               is_validated
> ....
> }
> is is_validated always true, can we add an Assert on it?

Sure.  FWIW the reason it's a parameter at all, is that the obvious next
patch is to add support for NOT VALID constraints.  I don't want to
introduce support for NOT VALID immediately with the first patch because
I'm sure some wrinkles will appear; but a followup patch will surely
follow shortly.

> in AddRelationNotNullConstraints
> for (int outerpos = 0; outerpos < list_length(old_notnulls); outerpos++)
> {
> }
> CookedConstraint struct already has "int inhcount;"
> can we rely on that, rather than using add_inhcount?
> we can also add an Assert: "Assert(!cooked->is_no_inherit);"

I'm not sure that works, because if your parent has two parents, you
don't want to add two -- you still have only one immediate parent.

I think the best way to check for correctness is to set up a scenario
where you would have that cooked->inhcount=2 (using whatever CREATE
TABLEs are necessary) and then see if ALTER TABLE NO INHERIT reach the
correct count (0) when all [immediate] parents are detached.  But
anyway, keep in mind that inhcount keeps the number of _immediate_
parents, not the number of ancestors.

>         /*
>          * Remember primary key index, if any.  We do this only if the index
>          * is valid; but if the table is partitioned, then we do it even if
>          * it's invalid.
>          *
>          * The reason for returning invalid primary keys for foreign tables is
>          * because of pg_dump of NOT NULL constraints, and the fact that PKs
>          * remain marked invalid until the partitions' PKs are attached to it.
>          * If we make rd_pkindex invalid, then the attnotnull flag is reset
>          * after the PK is created, which causes the ALTER INDEX ATTACH
>          * PARTITION to fail with 'column ... is not marked NOT NULL'.  With
>          * this, dropconstraint_internal() will believe that the columns must
>          * not have attnotnull reset, so the PKs-on-partitions can be attached
>          * correctly, until finally the PK-on-parent is marked valid.
>          *
>          * Also, this doesn't harm anything, because rd_pkindex is not a
>          * "real" index anyway, but a RELKIND_PARTITIONED_INDEX.
>          */
>         if (index->indisprimary &&
>             (index->indisvalid ||
>              relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
>         {
>             pkeyIndex = index->indexrelid;
>             pkdeferrable = !index->indimmediate;
>         }
> The comment (especially paragraph "The reason for returning invalid
> primary keys") is overwhelming.
> Can you also add some sql examples into the comments.
> I guess some sql examples, people can understand it more easily?

Ooh, thanks for catching this -- this comment is a leftover from
previous idea that you could have PKs without NOT NULL.  I think it
mostly needs to be removed, and maybe the whole "if" clause put back to
its original form.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)