From: Chao Li <li.evan.chao@gmail.com>
Message-Id: <69CA7636-0359-42B6-B971-16A036698187@gmail.com>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_D9742CE3-0BC0-4353-841C-21EF37D07666"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes
Date: Wed, 13 May 2026 16:35:30 +0800
In-Reply-To: <70401.1778604528@localhost>
Cc: alvherre@kurilemu.de,
 Kirill Reshke <reshkekirill@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
To: Antonin Houska <ah@cybertec.at>
References: <agIBukr85PlM6um_@alvherre.pgsql> <70401.1778604528@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/69CA7636-0359-42B6-B971-16A036698187%40gmail.com>
Precedence: bulk


--Apple-Mail=_D9742CE3-0BC0-4353-841C-21EF37D07666
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On May 13, 2026, at 00:48, Antonin Houska <ah@cybertec.at> wrote:
>=20
> =C3=81lvaro Herrera <alvherre@kurilemu.de> wrote:
>=20
>> On 2026-May-11, Chao Li wrote:
>>=20
>>>> On May 10, 2026, at 06:38, =C3=81lvaro Herrera =
<alvherre@kurilemu.de> wrote:
>>=20
>>>> I think it would be a good idea to make identity_key_equal() not =
deform
>>>> all attributes, but instead only up to the last one it needs for =
the key
>>>> comparisons.
>>>=20
>>> That=E2=80=99s true. Please see v3.
>>=20
>> Thanks.  I did one further small change, namely to determine these =
last
>> attnums just once per run rather than once per tuple.  Pushed now.
>=20
> I appreciate that REPACK can handle more cases now! However, I found a =
problem
> (or at least a question) when rebasing the improvements for the next
> release(s). (It's related to splitting the table scan into multiple =
block
> ranges and use one snapshot per range, details are not too important =
here, )
> Assertion failure in the new code made me think if other than B-tree =
indexes
> should be allowed in the USING INDEX clause of REPACK.
>=20
> AFAICS, only B-tree indexes (and some special ones that don't appear =
in the
> core) provide ordering information - see get_relation_info():
>=20
> /*
>  * Fetch the ordering information for the index, if any.
>  */
> if (info->relam =3D=3D BTREE_AM_OID)
> {
> ...
> info->sortopfamily =3D info->opfamily;
> ...
> }
> else if (amroutine->amcanorder)
> {
> /*
>  * Otherwise, identify the corresponding btree opfamilies
>  * by trying to map this index's "<" operators into btree.
>  * Since "<" uniquely defines the behavior of a sort
>  * order, this is a sufficient test.
>  ...
> }
> else
> {
> ...
> info->sortopfamily =3D NULL;
> ...
> }
>=20
>=20
> Therefore, index scan shouldn't be possible for GIST index - see
> build_index_paths():
>=20
> index_is_ordered =3D (index->sortopfamily !=3D NULL);
>=20
>=20
> So I'm not sure if clustering makes sense here. What makes me confused =
is that
> GIST has IndexAmRoutine.amclusterable=3Dtrue. As it has =
amcanorder=3Dfalse at the
> same time, I suspect it might be just a thinko. However, if we simply =
set
> amclusterable to false, it can break upgrade to PG 19 for users who =
already
> "clustered" some table by a GIST index (for mysterious reasons). (BTW, =
do we
> need the amclusterable field at all?)
>=20
> REPACK currently rejects explicit sort if non-B-tree index is =
specified (due
> to lack of ordering information), but it still scans the index rather =
than
> the heap - see copy_table_data() and =
heapam_relation_copy_for_cluster().
>=20
> Does this seem worth fixing now? Or maybe at least worth some comments =
(unless
> I'm completely wrong)?

After some investigation, I think I see the mismatch:

* get_relation_info(): non-ordered GiST cannot provide sort order. That =
is expected.
* copy_table_data() only uses plan_cluster_use_sort() for btree. For any =
other clusterable index, it sets use_sort =3D false and does a raw index =
scan.
* The docs say REPACK can re-sort using index scan =E2=80=9Cif the index =
is a b-tree=E2=80=9D or seqscan+sort, which does not describe what the =
code actually does for GiST.

I am not sure whether we should change the behavior in PG19. Alvaro may =
have a better idea about that. But I agree that we can at least clarify =
the code comment and documentation. The attached patch attempts to do =
that.

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


--Apple-Mail=_D9742CE3-0BC0-4353-841C-21EF37D07666
Content-Disposition: attachment;
	filename=repack_comment.diff
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="repack_comment.diff"
Content-Transfer-Encoding: 7bit

diff --git a/doc/src/sgml/ref/repack.sgml b/doc/src/sgml/ref/repack.sgml
index 0cb72b6b289..c9ef358261e 100644
--- a/doc/src/sgml/ref/repack.sgml
+++ b/doc/src/sgml/ref/repack.sgml
@@ -78,11 +78,13 @@ REPACK [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] USING
 
    <para>
     If the <literal>USING INDEX</literal> clause is specified, the rows in
-    the table are stored in the order that the index specifies;
-    <firstterm>clustering</firstterm>, because rows are physically clustered
-    afterwards.
-    If an index name is specified in the command, the order implied by that
-    index is used, and that index is configured as the index to cluster on.
+    the table are physically rearranged according to the specified index;
+    this is known as <firstterm>clustering</firstterm>.  For b-tree indexes,
+    this means the table is stored in the index's sort order.  Other clusterable
+    index access methods use their own index scan order, which does not
+    necessarily correspond to a SQL sort order.
+    If an index name is specified in the command, that index is used for
+    clustering, and is configured as the index to cluster on.
     (This also applies to an index given to the <command>CLUSTER</command>
     command.)
     If no index name is specified, then the index that has
@@ -101,12 +103,12 @@ REPACK [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] USING
    <para>
     Clustering is a one-time operation: when the table is
     subsequently updated, the changes are not clustered.  That is, no attempt
-    is made to store new or updated rows according to their index order.  (If
-    one wishes, one can periodically recluster by issuing the command again.
-    Also, setting the table's <literal>fillfactor</literal> storage parameter
-    to less than 100% can aid in preserving cluster ordering during updates,
-    since updated rows are kept on the same page if enough space is available
-    there.)
+    is made to store new or updated rows according to the clustering order.
+    (If one wishes, one can periodically recluster by issuing the command
+    again.  Also, setting the table's <literal>fillfactor</literal> storage
+    parameter to less than 100% can aid in preserving cluster ordering during
+    updates, since updated rows are kept on the same page if enough space is
+    available there.)
    </para>
 
    <para>
@@ -123,11 +125,12 @@ REPACK [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] USING
    </para>
 
    <para>
-    <command>REPACK</command> can re-sort the table using either an index scan
-    on the specified index (if the index is a b-tree), or a sequential scan
-    followed by sorting.  It will attempt to choose the method that will be
-    faster, based on planner cost parameters and available statistical
-    information.
+    When clustering on a b-tree index, <command>REPACK</command> can rewrite
+    the table using either an index scan on the specified index, or a
+    sequential scan followed by sorting.  It will attempt to choose the method
+    that will be faster, based on planner cost parameters and available
+    statistical information.  When clustering on a non-b-tree index,
+    <command>REPACK</command> uses an index scan.
    </para>
 
    <para>
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 860e2aecbe9..6eb685b2240 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -1364,10 +1364,16 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex,
 
 	/*
 	 * Decide whether to use an indexscan or seqscan-and-optional-sort to scan
-	 * the OldHeap.  We know how to use a sort to duplicate the ordering of a
-	 * btree index, and will use seqscan-and-sort for that case if the planner
-	 * tells us it's cheaper.  Otherwise, always indexscan if an index is
-	 * provided, else plain seqscan.
+	 * the OldHeap.  For btree indexes, the scan order is a well-defined sort
+	 * order that can also be reproduced by an explicit sort, so use the
+	 * planner to choose between indexscan and seqscan-and-sort.
+	 *
+	 * Other index AMs can be marked clusterable even though they do not
+	 * provide btree-style ordering information to the planner.  For those,
+	 * clustering means rewriting the heap in the AM's index scan order, which
+	 * may improve locality but cannot be duplicated by sorting here, so leave
+	 * use_sort false.  If no index is provided, use a plain seqscan.
+
 	 */
 	if (OldIndex != NULL && OldIndex->rd_rel->relam == BTREE_AM_OID)
 		use_sort = plan_cluster_use_sort(RelationGetRelid(OldHeap),

--Apple-Mail=_D9742CE3-0BC0-4353-841C-21EF37D07666--