public inbox for [email protected]  

help / color / mirror / Atom feed

From: Michael Paquier <[email protected]>
To: Chao Li <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
Date: Tue, 17 Mar 2026 07:57:57 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com>
	<[email protected]>
	<CAN4CZFPX_t9q4W=jTh4jW4TOz=Xhfv0gPi9jG0MfXv9Ek7BO5w@mail.gmail.com>
	<[email protected]>
	<CAN4CZFMjyPYqNY6qD4ArRZKUyC4oEPAfu-OesfChGMwMq1CG_w@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

On Mon, Mar 16, 2026 at 05:07:51PM +0800, Chao Li wrote:
> Basically, 0002 does the same thing as 0001 just on a different
> sub-command of ALTER TABLE.

        case AT_DropInherit:    /* NO INHERIT */
[...]
-           /* No command-specific prep needed */
+           ATPrepChangeInherit(rel);

This change means that we are plugging in earlier a check based on a
typed table for the NO INHERIT case.  This sequence fails already on
HEAD and with the patch, but generates a different error in the last
query between HEAD and the patch, and is not covered by your patch:
CREATE TYPE person_type AS (id int, name text);
CREATE TABLE persons OF person_type;
CREATE TABLE stuff (a int);
ALTER TABLE persons NO INHERIT stuff;

I'd suggest the addition of a test in typed_table.sql, just after the
"ALTER TABLE persons INHERIT stuff;".  The INHERIT case is already
blocked, so NO INHERIT is a no-op anyway because we complain about the
typed table not being inherited.  How about doing that as a separate
patch, with the second patch for tablescmds.c updating the regression
test output?  I thought that the NO INHERIT command was allowed, but
we fail, so blocking it with a different, somewhat clearer error is OK
by me.

You are right that the comment on top of ATPrepAddInherit() is wrong,
and that we'd better clean it up.  The code does not do what the
comment tells.  That does not seem worth troubling stable branches.

A second thing is that we'd better add an assertion in
ATExecDropInherit() to make sure that this code path is never taken
for a RELKIND_PARTITIONED_TABLE, ensuring that the prep phase blocked
this case?
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, 2-signature.asc)
  download

view thread (6+ 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: <[email protected]>

* 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