Re: ALTER TABLE ONLY .. DROP CONSTRAINT on partitioned tables

Hi Alvaro,

On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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