Re: not null constraints, again

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)