Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix REPACK with WITHOUT OVERLAPS replica identity indexes

  2. Remove working test that was supposed to fail

  1. Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Chao Li <li.evan.chao@gmail.com> — 2026-05-08T04:21:53Z

    Hi,

While testing UPDATE FOR PORTION OF, I started wondering whether REPACK supports temporal tables. In theory, it should, because temporal WITHOUT OVERLAPS indexes can be used as replica identity indexes. So I created a test script, repack_temporal.spec, which is included in the attached patch, and it failed.

I found that REPACK hard-codes BTEqualStrategyNumber when calling get_opfamily_member(). That seems wrong, because build_replindex_scan_key() uses IndexAmTranslateCompareType() to get the equality strategy for COMPARE_EQ.

After fixing the hard-coded BTEqualStrategyNumber, the temporal test passed. Then I added another test for multirange, repack_temporal_multirange.spec, which also failed. The reason is that find_target_tuple() uses the identity index to find the first tuple and returns it directly, but a lossy index scan may return false positives and require recheck.

Please see the attached patch for the fix details and test scripts.

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

  2. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Kirill Reshke <reshkekirill@gmail.com> — 2026-05-08T17:47:39Z

    On Fri, 8 May 2026 at 09:22, Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi,
>
> While testing UPDATE FOR PORTION OF, I started wondering whether REPACK supports temporal tables. In theory, it should, because temporal WITHOUT OVERLAPS indexes can be used as replica identity indexes. So I created a test script, repack_temporal.spec, which is included in the attached patch, and it failed.
>
> I found that REPACK hard-codes BTEqualStrategyNumber when calling get_opfamily_member(). That seems wrong, because build_replindex_scan_key() uses IndexAmTranslateCompareType() to get the equality strategy for COMPARE_EQ.
>
> After fixing the hard-coded BTEqualStrategyNumber, the temporal test passed. Then I added another test for multirange, repack_temporal_multirange.spec, which also failed. The reason is that find_target_tuple() uses the identity index to find the first tuple and returns it directly, but a lossy index scan may return false positives and require recheck.
>
> Please see the attached patch for the fix details and test scripts.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>

your analysis appears correct to me

>  + while (index_getnext_slot(scan, ForwardScanDirection, retrieved))
> + {
> + if (scan->xs_recheck && !identity_key_equal(chgcxt, locator, retrieved))
> + continue;
> +
> + retval = true;
> + break;
> + }

Should we add CFI() ?


Also, do we really need isolation tests and inj points here? Doesn't a
simple regression test for REPACK execute the same code?


-- 
Best regards,
Kirill Reshke

  3. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Chao Li <li.evan.chao@gmail.com> — 2026-05-09T08:36:34Z

    
> On May 9, 2026, at 01:47, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> On Fri, 8 May 2026 at 09:22, Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>> Hi,
>> 
>> While testing UPDATE FOR PORTION OF, I started wondering whether REPACK supports temporal tables. In theory, it should, because temporal WITHOUT OVERLAPS indexes can be used as replica identity indexes. So I created a test script, repack_temporal.spec, which is included in the attached patch, and it failed.
>> 
>> I found that REPACK hard-codes BTEqualStrategyNumber when calling get_opfamily_member(). That seems wrong, because build_replindex_scan_key() uses IndexAmTranslateCompareType() to get the equality strategy for COMPARE_EQ.
>> 
>> After fixing the hard-coded BTEqualStrategyNumber, the temporal test passed. Then I added another test for multirange, repack_temporal_multirange.spec, which also failed. The reason is that find_target_tuple() uses the identity index to find the first tuple and returns it directly, but a lossy index scan may return false positives and require recheck.
>> 
>> Please see the attached patch for the fix details and test scripts.
>> 
>> Best regards,
>> --
>> Chao Li (Evan)
>> HighGo Software Co., Ltd.
>> https://www.highgo.com/
>> 
> 
> your analysis appears correct to me

Hi Kirill, thanks for your review.

> 
>> + while (index_getnext_slot(scan, ForwardScanDirection, retrieved))
>> + {
>> + if (scan->xs_recheck && !identity_key_equal(chgcxt, locator, retrieved))
>> + continue;
>> +
>> + retval = true;
>> + break;
>> + }
> 
> Should we add CFI() ?
> 

Oh, I didn’t consider that at all, because I thought there should not be a lot of candidate rows needing recheck. I am okay to add that.

> 
> Also, do we really need isolation tests and inj points here?

I think so. Without the injection point, the first phase of copying a new heap would be very fast, it would be hard to run an update in the second session. I think that’s way the repack code intentionally added an injection point before the first round of replay:

```
    /*
* During testing, wait for another backend to perform concurrent data
* changes which we will process below.
*/
INJECTION_POINT("repack-concurrently-before-lock", NULL);
```

> Doesn't a
> simple regression test for REPACK execute the same code?
> 

It seems we intentionally avoid to run repack test in the regress test, see [1] and [2].

PFA v2: added the CFI as Kirill suggested.

[1] https://postgr.es/m/769631.1777575242@sss.pgh.pa.us
[2] https://git.postgresql.org/cgit/postgresql.git/commit/?id=2fd787d0aac1cb00a42ebce92ebb1d7534035ee3

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

  4. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-05-09T22:38:08Z

    On 2026-May-09, Chao Li wrote:

> > On May 9, 2026, at 01:47, Kirill Reshke <reshkekirill@gmail.com> wrote:
> > 
> > On Fri, 8 May 2026 at 09:22, Chao Li <li.evan.chao@gmail.com> wrote:

> >> While testing UPDATE FOR PORTION OF, I started wondering whether
> >> REPACK supports temporal tables. In theory, it should, because
> >> temporal WITHOUT OVERLAPS indexes can be used as replica identity
> >> indexes. So I created a test script, repack_temporal.spec, which is
> >> included in the attached patch, and it failed.

Nice find, thanks for testing.

> >> I found that REPACK hard-codes BTEqualStrategyNumber when calling
> >> get_opfamily_member(). That seems wrong, because
> >> build_replindex_scan_key() uses IndexAmTranslateCompareType() to
> >> get the equality strategy for COMPARE_EQ.

Makes sense.

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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  5. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Chao Li <li.evan.chao@gmail.com> — 2026-05-11T06:54:18Z

    
> On May 10, 2026, at 06:38, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> On 2026-May-09, Chao Li wrote:
> 
>>> On May 9, 2026, at 01:47, Kirill Reshke <reshkekirill@gmail.com> wrote:
>>> 
>>> On Fri, 8 May 2026 at 09:22, Chao Li <li.evan.chao@gmail.com> wrote:
> 
>>>> While testing UPDATE FOR PORTION OF, I started wondering whether
>>>> REPACK supports temporal tables. In theory, it should, because
>>>> temporal WITHOUT OVERLAPS indexes can be used as replica identity
>>>> indexes. So I created a test script, repack_temporal.spec, which is
>>>> included in the attached patch, and it failed.
> 
> Nice find, thanks for testing.
> 
>>>> I found that REPACK hard-codes BTEqualStrategyNumber when calling
>>>> get_opfamily_member(). That seems wrong, because
>>>> build_replindex_scan_key() uses IndexAmTranslateCompareType() to
>>>> get the equality strategy for COMPARE_EQ.
> 
> Makes sense.
> 
> 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.

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

  6. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-05-11T16:21:17Z

    On 2026-May-11, Chao Li wrote:

> > On May 10, 2026, at 06:38, Álvaro Herrera <alvherre@kurilemu.de> 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.

Thanks for testing!

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

  7. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Chao Li <li.evan.chao@gmail.com> — 2026-05-12T02:31:05Z

    
> On May 12, 2026, at 00:21, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> On 2026-May-11, Chao Li wrote:
> 
>>> On May 10, 2026, at 06:38, Álvaro Herrera <alvherre@kurilemu.de> 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.
> 
> Thanks for testing!
> 
> -- 
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Hi Álvaro, thank you so much for tuning the code and pushing.

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

  8. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Antonin Houska <ah@cybertec.at> — 2026-05-12T16:48:48Z

    Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2026-May-11, Chao Li wrote:
> 
> > > On May 10, 2026, at 06:38, Álvaro Herrera <alvherre@kurilemu.de> 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)?

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  9. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Chao Li <li.evan.chao@gmail.com> — 2026-05-13T08:35:30Z

    
> On May 13, 2026, at 00:48, Antonin Houska <ah@cybertec.at> wrote:
> 
> Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
>> On 2026-May-11, Chao Li wrote:
>> 
>>>> On May 10, 2026, at 06:38, Álvaro Herrera <alvherre@kurilemu.de> 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/

  10. Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes

    Antonin Houska <ah@cybertec.at> — 2026-05-13T12:00:38Z

    Chao Li <li.evan.chao@gmail.com> wrote:

> 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.

I meant just a comment that reminds developers that something needs to be
fixed, rather than defending the inconsistent behavior.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com