MIME-Version: 1.0
References: <CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com>
 <CAEoWx2k5800OjHO5KhTDv4JFYpDmyfh3MTtuB=7PP-0hLmG8yQ@mail.gmail.com>
 <07773235-2E94-478F-BEF6-38C73B0553B8@gmail.com> <EA6CB317-60C2-4BA9-8E02-DBCAD384F107@gmail.com>
In-Reply-To: <EA6CB317-60C2-4BA9-8E02-DBCAD384F107@gmail.com>
From: Zsolt Parragi <zsolt.parragi@percona.com>
Date: Fri, 23 Jan 2026 07:43:07 +0000
Message-ID: <CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com>
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
To: Chao Li <li.evan.chao@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS%2Bw%40mail.gmail.com>
Precedence: bulk

Hello!

A simple patch and generally looks good, I only have a few observations.

> =E2=80=9CALTER TABLE =E2=80=A6 CLUSTER ON=E2=80=9D and "SET WITHOUT CLUST=
ER" are not supported for
> partitioned tables, but currently ATPrepCmd() allows them through and the=
y
> 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 cha=
nge.

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=3D# 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.