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
