public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Antonin Houska <[email protected]>
Cc: [email protected]
Cc: Kirill Reshke <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes
Date: Wed, 13 May 2026 16:35:30 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <70401.1778604528@localhost>
References: <[email protected]>
<70401.1778604528@localhost>
> On May 13, 2026, at 00:48, Antonin Houska <[email protected]> wrote:
>
> Álvaro Herrera <[email protected]> wrote:
>
>> On 2026-May-11, Chao Li wrote:
>>
>>>> On May 10, 2026, at 06:38, Álvaro Herrera <[email protected]> wrote:
>>
>>>> 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.
>>>
>>> That’s true. Please see v3.
>>
>> Thanks. I did one further small change, namely to determine these last
>> attnums just once per run rather than once per tuple. Pushed now.
>
> 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.
>
> AFAICS, only B-tree indexes (and some special ones that don't appear in the
> core) provide ordering information - see get_relation_info():
>
> /*
> * Fetch the ordering information for the index, if any.
> */
> if (info->relam == BTREE_AM_OID)
> {
> ...
> info->sortopfamily = 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 = NULL;
> ...
> }
>
>
> Therefore, index scan shouldn't be possible for GIST index - see
> build_index_paths():
>
> index_is_ordered = (index->sortopfamily != NULL);
>
>
> So I'm not sure if clustering makes sense here. What makes me confused is that
> GIST has IndexAmRoutine.amclusterable=true. As it has amcanorder=false 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?)
>
> 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().
>
> 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 = false and does a raw index scan.
* The docs say REPACK can re-sort using index scan “if the index is a b-tree” 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/
Attachments:
[application/octet-stream] repack_comment.diff (4.6K, 2-repack_comment.diff)
download | inline diff:
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),
view thread (10+ 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], [email protected]
Subject: Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes
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