public inbox for [email protected]  

help / color / mirror / Atom feed

From: Zsolt Parragi <[email protected]>
To: Chao Li <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
Date: Fri, 23 Jan 2026 07:43:07 +0000
Message-ID: <CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com>
	<CAEoWx2k5800OjHO5KhTDv4JFYpDmyfh3MTtuB=7PP-0hLmG8yQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>

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.

view thread (11+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
  In-Reply-To: <CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox