Date: Thu, 6 Apr 2023 13:20:41 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
Message-ID: <ZC8NeXrN1yRH1OzS@telsasoft.com>
References: <a3641b38-4547-fd26-855b-045e2736672a@enterprisedb.com>
 <20230405233356.qs4w4jtfc3kq4obl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230405233356.qs4w4jtfc3kq4obl@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/ZC8NeXrN1yRH1OzS%40telsasoft.com>
Precedence: bulk

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