From: Chao Li <li.evan.chao@gmail.com>
Content-Type: text/plain;
	charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.300.41.1.7\))
Subject: Re: tablecmds: reject CLUSTER ON for partitioned tables earlier
Date: Wed, 28 Jan 2026 09:56:36 +0800
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>
 <CAN4CZFMUJWM0veLK93Ew8mYaL5pcU6G550STh6AZ-_LWV2zS+w@mail.gmail.com>
 <16D5D52A-1B99-4371-982E-257C195D2924@gmail.com>
 <CAN4CZFPX_t9q4W=jTh4jW4TOz=Xhfv0gPi9jG0MfXv9Ek7BO5w@mail.gmail.com>
 <5244008D-79E1-484B-9407-21F5D388EC7F@gmail.com>
 <aXhty3YQceFniabz@paquier.xyz>
To: Michael Paquier <michael@paquier.xyz>,
 Zsolt Parragi <zsolt.parragi@percona.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
In-Reply-To: <aXhty3YQceFniabz@paquier.xyz>
Message-Id: <A1847A7B-7CB8-4A03-8CEA-FF7D3991DEDA@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/A1847A7B-7CB8-4A03-8CEA-FF7D3991DEDA%40gmail.com>
Precedence: bulk


> On Jan 27, 2026, at 15:48, Michael Paquier <michael@paquier.xyz> =
wrote:
>=20
> On Tue, Jan 27, 2026 at 07:13:04AM +0800, Chao Li wrote:
>> I added two new test cases in 0002 that trigger the check.
>>=20
>> 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?
>=20
> There is more to this set of changes than it looks at first sight.
>=20
> Hence, one question about 0001: can the previous error path in
> mark_index_clustered() be reached through a different mean than ALTER
> TABLE?  If yes, we should have a test for it.  If no, it could be
> switched to an elog(ERROR) or an assertion.  The code paths leading to
> the previous error should be analyzed further.

Okay, I spent time today investigating mark_index_clustered() today.

First, I reset the source tree to 05fb5d6 where the partitioned table =
check was added to mark_index_clustered(). The commit subject is "Ignore =
partitioned indexes where appropriate=E2=80=9D. It added the check in 3 =
functions:

 * cluster()
 * mark_index_clustered()
 * get_relation_info() - not in scope of this discussion

At this commit, ALTER TABLE =E2=80=A6 CLUSTER ON / SET WITHOUT CLUSTER =
code patch could reach mark_index_clustered(). Other than that, =
mark_index_clustered() was only called by rebuild_relation() when the =
parameter indexOid is valid; rebuild_relation() was only called by =
cluster_rel(); and cluster_rel() was called by vacuum_rel() and =
cluster().=20

 * For the cluster() code patch: because of the check added to cluster() =
by this commit, partitioned table would return early, thus =
mark_index_clustered() was actually not called.
 * For the vacuum_rel() code path: there was already a check for =
partitioned table to return early, thus cluster_rel() won=E2=80=99t be =
called against partitioned tables, so that mark_index_clustered() could =
not be called either.

So, looks like the check added to mark_index_clustered() was only for =
the ALTER TABLE code path.

Then, switching the source tree back to this patch. Now, for the ALTER =
TABLE code path, 0001 ensures partitioned table won=E2=80=99t reach =
mark_index_clustered().

Other than ALTER TABLE, mark_index_clustered() is only called by =
rebuild_relation(); rebuild_relation() is only called by cluster_rel(); =
cluster_rel() is called by vacuum_rel() and cluster(). So, the call =
paths are the same as commit 05fb5d6.

For the cluster() code path, I traced this scenario:
```
evantest=3D# create table p (id int) partition by range (id);
CREATE TABLE
evantest=3D# create table p1 partition of p for values from (0) to (10);
CREATE TABLE
evantest=3D# create index p_idx on p (id);
CREATE INDEX
evantest=3D# cluster p using p_idx;
CLUSTER
```
The code ran into cluster(). Now, cluster() is much complicated than it =
was in commit 05fb5d6. For a partitioned table, it iterates all leaf =
partitions to call cluster_rel():
```
	/*
	 * Either we're processing a partitioned table, or we were not =
given any
	 * table name at all.  In either case, obtain a list of =
relations to
	 * process.
	 *
	 * In the former case, an index name must have been given, so we =
don't
	 * need to recheck its "indisclustered" bit, but we have to =
check that it
	 * is an index that we can cluster on.  In the latter case, we =
set the
	 * option bit to have indisclustered verified.
	 *
	 * Rechecking the relation itself is necessary here in all =
cases.
	 */
	params.options |=3D CLUOPT_RECHECK;
	if (rel !=3D NULL)
	{
		Assert(rel->rd_rel->relkind =3D=3D =
RELKIND_PARTITIONED_TABLE);
		check_index_is_clusterable(rel, indexOid, =
AccessShareLock);
		rtcs =3D =
get_tables_to_cluster_partitioned(cluster_context, indexOid);

		/* close relation, releasing lock on parent table */
		table_close(rel, AccessExclusiveLock);
	}
        =E2=80=A6.
        cluster_multiple_rels(rtcs, &params); // cluster_rel() is called =
here
```
So, partitioned table should never reach mark_index_clustered() from the =
cluster() code patch.

For the vacuum_rel() code patch, same as before, partitioned table will =
return early, cluster_rel() won=E2=80=99t be called at all:
```
	/*
	 * Silently ignore partitioned tables as there is no work to be =
done.  The
	 * useful work is on their child partitions, which have been =
queued up for
	 * us separately.
	 */
	if (rel->rd_rel->relkind =3D=3D RELKIND_PARTITIONED_TABLE)
	{
		relation_close(rel, lmode);
		PopActiveSnapshot();
		CommitTransactionCommand();
		/* It's OK to proceed with ANALYZE on this table */
		return true;
	}
```

In summary, with 0001, the partitioned table check in =
mark_index_clustered() is no longer needed. But as =
mark_index_clustered() is an extern-ed function, it might have future =
callers, I think we can change ereport(ERROR) to an Assert(). I will =
include the change in next revision.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/