Re: cataloguing NOT NULL constraints - Mailing list pgsql-hackers

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