From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
Date: Thu, 6 Apr 2023 13:20:41 -0500
Message-ID: <ZC8NeXrN1yRH1OzS@telsasoft.com> (raw)
In-Reply-To: <20230405233356.qs4w4jtfc3kq4obl@alvherre.pgsql>
References: <a3641b38-4547-fd26-855b-045e2736672a@enterprisedb.com>
<20230405233356.qs4w4jtfc3kq4obl@alvherre.pgsql>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, alvherre@alvh.no-ip.org, peter.eisentraut@enterprisedb.com, pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
In-Reply-To: <ZC8NeXrN1yRH1OzS@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox