Re: cataloguing NOT NULL constraints

On Thu, Apr 06, 2023 at 01:33:56AM +0200, Alvaro Herrera wrote:
> - The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>),
> + The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>, and
> + except for the <literal>NOT NULL <replaceable>column_name</replaceable></literal>
> + form to add a table constraint),

The "except" part seems pretty incoherent to me :(

> + if (isnull)
> + elog(ERROR, "null conkey for NOT NULL constraint %u", conForm->oid);

could use SysCacheGetAttrNotNull()

> + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("cannot add constraint to only the partitioned table when partitions exist"),
> + errhint("Do not specify the ONLY keyword."));
> + else
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("cannot add constraint to table with inheritance children"),

missing "only" ?

> + conrel = table_open(ConstraintRelationId, RowExclusiveLock);

Should this be opened after the following error check ?

> + arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */
> + numkeys = ARR_DIMS(arr)[0];
> + if (ARR_NDIM(arr) != 1 ||
> + numkeys < 0 ||
> + ARR_HASNULL(arr) ||
> + ARR_ELEMTYPE(arr) != INT2OID)
> + elog(ERROR, "conkey is not a 1-D smallint array");
> + attnums = (int16 *) ARR_DATA_PTR(arr);
> +
> + for (int i = 0; i < numkeys; i++)
> + unconstrained_cols = lappend_int(unconstrained_cols, attnums[i]);
> + }

Does "arr" need to be freed ?

> + * Since the above deletion has been made visible, we can now
> + * search for any remaining constraints on this column (or these
> + * columns, in the case we're dropping a multicol primary key.)
> + * Then, verify whether any further NOT NULL or primary key exist,

If I'm reading it right, I think it should say "exists"

> +/*
> + * When a primary key index on a partitioned table is to be attached an index
> + * on a partition, the partition's columns should also be marked NOT NULL.
> + * Ensure that is the case.

I think the comment may be missing words, or backwards.
The index on the *partitioned* table wouldn't be attached.
Is the index on the *partition* that's attached *to* the former index.

> +create table c() inherits(inh_p1, inh_p2, inh_p3, inh_p4);
> +NOTICE: merging multiple inherited definitions of column "f1"
> +NOTICE: merging multiple inherited definitions of column "f1"
> +ERROR: relation "c" already exists

Do you intend to make an error here ?

Also, I think these table names may be too generic, and conflict with
other parallel tests, now or in the future.

> +create table d(a int not null, f1 int) inherits(inh_p3, c);
> +ERROR: relation "d" already exists

And here ?

> +-- with explicitely specified not null constraints

sp: explicitly

--
Justin