| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Zsolt Parragi <zsolt(dot)parragi(at)percona(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-26 23:13:04 |
| Message-ID: | 5244008D-79E1-484B-9407-21F5D388EC7F@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Jan 27, 2026, at 01:26, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> +ALTER TABLE nonpartitioned INHERIT partitioned; -- ok
> ERROR: cannot inherit from partitioned table "partitioned"
> -- cannot add NO INHERIT constraint to partitioned tables
>
> That comment should be fail
Fixed.
>
> Otherwise the patches look good.
>
Thanks a lot for confirming.
> The rest is about the two checks that seem redundant to me - I don't
> have a problem with leaving them as is, but they do seem redundant to
> me.
>
>> So, I would leave the check there, maybe use a separate discussion for removal of the check.
>
> I tried to find a way to trigger it and couldn't figure out anything,
> to me it seems unreachable.
>
>> However, there is a call path: vacuum -> vacuum_rel -> cluster_rel -> rebuild_relation -> mark_index_clustered. I am not sure if the check plays some role there.
>
> VACUUM FULL always passes InvalidOid to the cluster_rel for the index
> parameter, so we can't hit the error.
>
> CLUSTER is more difficult to follow, but to me that also seems like to
> never hit this error, and the behavior I see is also described in the
> documentation (mark_index_clustered is only called for leaf
> partitions, where it works). Following the calls in the code also
> shows the same to me, that this method is now only called for
> partitions.
I will need more investigation on this, so let’s leave it for a seperate discussion.
>
>> No, the check is not redundant. It checks for child partitions, while ATPrepChangeInherit only blocks partitioned tables.
>
> And I have the same issue with this one: I modified that error in
> ATExecDropInherit to an assertion locally. The test suite had no new
> failures, I also tried to write a few tests manually, but I wasn't
> able to trigger it. Maybe I'm missing something, but I think it's
> redundant now.
I added two new test cases in 0002 that trigger the check.
BTW, this is the CF entry: https://commitfest.postgresql.org/patch/6415/. You may mark yourself as a reviewer, and once you consider the patch is ready to go, would you mind change the status to Ready For Committer?
PFA v6.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| Attachment | Content-Type | Size |
|---|---|---|
| v6-0001-tablecmds-reject-CLUSTER-ON-for-partitioned-table.patch | application/octet-stream | 2.7 KB |
| v6-0002-tablecmds-reject-INHERIT-NO-INHERIT-for-partition.patch | application/octet-stream | 6.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andres Freund | 2026-01-26 23:25:44 | Re: unnecessary executor overheads around seqscans |
| Previous Message | Sami Imseih | 2026-01-26 22:48:59 | Re: Cleaning up PREPARE query strings? |