| From: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: tablecmds: reject CLUSTER ON for partitioned tables earlier |
| Date: | 2026-01-23 07:43:07 |
| Message-ID: | CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello!
A simple patch and generally looks good, I only have a few observations.
> “ALTER TABLE … CLUSTER ON” and "SET WITHOUT CLUSTER" are not supported for
> partitioned tables, but currently ATPrepCmd() allows them through and they
> only fail later at execution time.
Looking at the ALTER TABLE documentation, for other options there is a
mention like "This form is not currently supported on partitioned
tables." / "This form is not supported for partitioned tables."
I don't see this mentioned for CLUSTER or INHERIT. Maybe it would be
better to also mention this in the documentation?
Also, there seems to be no test for partitioned NO INHERIT, since the
patch changes it, it could also add a test case to verify the
behavior.
rg "INHERIT" | grep "cannot be performed"
src/test/regress/expected/alter_table.out:ERROR: ALTER action INHERIT
cannot be performed on relation "partitioned"
rg "NO INHERIT" | grep "cannot be performed"
# no result
tablecmds.c:5202
case AT_DropInherit: /* NO INHERIT */
ATSimplePermissions(cmd->subtype, rel,
- ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
+ ATT_TABLE | ATT_FOREIGN_TABLE);
/* This command never recurses */
+ ATPrepChangeInherit(rel);
/* No command-specific prep needed */
That last comment seems to be a leftover, it's no longer true with this change.
tablecmds.c:17289 trailing whitespace (in the empty line)
/*
+ * ALTER TABLE INHERIT
+ *
+ * Add a parent to the child's parents. This verifies that all the columns and
+ * check constraints of the parent appear in the child and that they have the
+ * same data types and expressions.
+ *
* Return the address of the new parent relation.
*/
tablecmds.c:17860 - this check in ATExecDropInherit is now redundant,
since we already have it in ATPrepChangeInherit
> Before the patch:
> ```
> evantest=# ALTER TABLE p_test CLUSTER ON idx_p_test_id;
> ERROR: cannot mark index clustered in partitioned table
Can we still reach the original error in mark_index_clustered somehow?
I don't see any examples in the test suite, or execution paths when I
have looked at the code, and it can be confusing to see a different
error code/message there.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-01-23 07:53:06 | Re: tablecmds: reject CLUSTER ON for partitioned tables earlier |
| Previous Message | Michael Paquier | 2026-01-23 07:03:59 | Some tests for TOAST, STORAGE MAIN/EXTENDED |