ALTER TABLE ONLY .. DROP CONSTRAINT on partitioned tables

Hi Alvaro,

On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1] that I
think is mistaken. Consider the following example:

CREATE TABLE parted (a int CONSTRAINT the_check CHECK (a > 0)) PARTITION BY LIST (a);
CREATE TABLE parted_1 PARTITION OF parted FOR VALUES IN (1);
ALTER TABLE ONLY parted DROP CONSTRAINT the_check;

The ALTER TABLE fails with the following message:

ERROR: cannot remove constraint from only the partitioned table when partitions exist
HINT: Do not specify the ONLY keyword.

and the relevant code in ATExecDropConstraint is:

/*
* For a partitioned table, if partitions exist and we are told not to
* recurse, it's a user error. It doesn't make sense to have a constraint
* be defined only on the parent, especially if it's a partitioned table.
*/
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
children != NIL && !recurse)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
errhint("Do not specify the ONLY keyword.")));

Note that the comment here is confused: it talks about a constraint that
would "be defined only on the parent", but that's bogus: the end result
would be that the constraint no longer exist on the parent but would
continue to exist on the children. Indeed it's not entirely
unimaginable that you start with a partitioned table with a bunch of
constraints which are enforced on all partitions, then you later decide
that you want this constraint to apply only to some of the partitions,
not the whole partitioned table. To implement that, you would drop the
constraint on the parent using ONLY, then drop it on a few of the
partitions, but still keep it on the other partitions. This would work
just fine if not for this ereport(ERROR).

Also, you can achieve the same end result by creating the constraint on
only some of the partitions and not on the partitioned table to start
with.

This also applies to ALTER TABLE ONLY ... DROP NOT NULL.

Of course, *adding* a constraint in this fashion is also forbidden, but
that makes perfect sense. Both restrictions were added as part of the
same commit, so I suppose we thought they were symmetrical behaviors and
failed to notice they weren't.

The DROP of such constraints can already be done on a table with legacy
inheritance children; it's just partitioned tables that have this
weirdness.

Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.

So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.

It doesn't surprise me that nobody has reported this inconsistency,
because it seems an unusual enough situation. For the same reason, I
wouldn't propose to backpatch this change. But I put forward the
attached patch, which removes the ereport(ERROR)s.

The patch looks good to me.

--
Thanks, Amit Langote

Hello,

On 2024-Sep-27, Amit Langote wrote:

On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1] that I
think is mistaken.

Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.

Right.

So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.

Thanks for looking! I have pushed the removal now.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
<inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell
<crab> inflex: you know that "amalgam" means "mixture with mercury",
more or less, right?
<crab> i.e., "deadly poison"

On Mon, Sep 30, 2024 at 6:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

On 2024-Sep-27, Amit Langote wrote:

On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1] that I
think is mistaken.

Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.

Right.

So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.

Thanks for looking! I have pushed the removal now.

works fine with check constraint, drop not null.
but seems not with primary key/unique constraint.

drop table if exists idxpart, idxpart0 cascade;
create table idxpart (a int not null primary key, constraint nc check
(a > 1)) partition by range (a);
create table idxpart0 (a int not null primary key, constraint nc check (a > 1));
alter table idxpart attach partition idxpart0 for values from (0) to (1000);
alter table only idxpart drop constraint nc;
alter table only idxpart drop constraint idxpart_pkey;

in the above case:
alter table only idxpart drop constraint idxpart_pkey;
is logical equivalent to
alter table idxpart drop constraint idxpart_pkey;

not sure this is desired behavior.