public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Álvaro Herrera <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Fix REPACK with WITHOUT OVERLAPS replica identity indexes
Date: Mon, 11 May 2026 14:54:18 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
> On May 10, 2026, at 06:38, Álvaro Herrera <[email protected]> wrote:
>
> On 2026-May-09, Chao Li wrote:
>
>>> On May 9, 2026, at 01:47, Kirill Reshke <[email protected]> wrote:
>>>
>>> On Fri, 8 May 2026 at 09:22, Chao Li <[email protected]> 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/
Attachments:
[application/octet-stream] v3-0001-Fix-REPACK-with-WITHOUT-OVERLAPS-replica-identity.patch (16.8K, 2-v3-0001-Fix-REPACK-with-WITHOUT-OVERLAPS-replica-identity.patch)
download | inline diff:
From 816c41624a7f6eff03aa7d499e3d84d501fb8352 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 8 May 2026 11:40:18 +0800
Subject: [PATCH v3] Fix REPACK with WITHOUT OVERLAPS replica identity indexes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
REPACK replay builds scan keys for the replica identity index, but it
hard-coded BTEqualStrategyNumber when looking up the equality operator.
That is not correct for non-btree identity indexes, such as the GiST
indexes created for WITHOUT OVERLAPS primary keys. In addition,
find_target_tuple() accepted the first tuple returned by the identity
index scan, which is unsafe for lossy index scans because the index AM may
return false positives with xs_recheck set.
Fix this by using IndexAmTranslateCompareType() to translate COMPARE_EQ
to the equality strategy number for the index AM, and by continuing the
scan when recheck is required until a candidate tuple matches the locator
tuple on all replica identity key columns.
The recheck uses the same equality operator functions as the identity
index scan keys, preserving ScanKey argument ordering.
Author: Chao Li <[email protected]>
Reviewed-by: Kirill Reshke <[email protected]>
Reviewed-by: Álvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/repack.c | 75 ++++++++++++-
src/test/modules/injection_points/Makefile | 2 +
.../expected/repack_temporal.out | 68 ++++++++++++
.../expected/repack_temporal_multirange.out | 74 +++++++++++++
src/test/modules/injection_points/meson.build | 2 +
.../specs/repack_temporal.spec | 90 ++++++++++++++++
.../specs/repack_temporal_multirange.spec | 102 ++++++++++++++++++
7 files changed, 409 insertions(+), 4 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/repack_temporal.out
create mode 100644 src/test/modules/injection_points/expected/repack_temporal_multirange.out
create mode 100644 src/test/modules/injection_points/specs/repack_temporal.spec
create mode 100644 src/test/modules/injection_points/specs/repack_temporal_multirange.spec
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 9a199dd9bfb..d56f22069f5 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -182,6 +182,9 @@ static void adjust_toast_pointers(Relation relation, TupleTableSlot *dest,
static bool find_target_tuple(Relation rel, ChangeContext *chgcxt,
TupleTableSlot *locator,
TupleTableSlot *retrieved);
+static bool identity_key_equal(ChangeContext *chgcxt,
+ TupleTableSlot *locator,
+ TupleTableSlot *candidate);
static void process_concurrent_changes(XLogRecPtr end_of_wal,
ChangeContext *chgcxt,
bool done);
@@ -2807,7 +2810,7 @@ find_target_tuple(Relation rel, ChangeContext *chgcxt, TupleTableSlot *locator,
{
Form_pg_index idx = chgcxt->cc_ident_index->rd_index;
IndexScanDesc scan;
- bool retval;
+ bool retval = false;
/*
* Scan key is passed by caller, so it does not have to be constructed
@@ -2829,12 +2832,69 @@ find_target_tuple(Relation rel, ChangeContext *chgcxt, TupleTableSlot *locator,
scan = index_beginscan(rel, chgcxt->cc_ident_index, GetActiveSnapshot(),
NULL, chgcxt->cc_ident_key_nentries, 0, 0);
index_rescan(scan, chgcxt->cc_ident_key, chgcxt->cc_ident_key_nentries, NULL, 0);
- retval = index_getnext_slot(scan, ForwardScanDirection, retrieved);
+ while (index_getnext_slot(scan, ForwardScanDirection, retrieved))
+ {
+ if (scan->xs_recheck && !identity_key_equal(chgcxt, locator, retrieved))
+ {
+ CHECK_FOR_INTERRUPTS();
+ continue;
+ }
+
+ retval = true;
+ break;
+ }
index_endscan(scan);
return retval;
}
+/*
+ * Check whether the candidate tuple matches the locator tuple on all replica
+ * identity key columns, using the same equality operators as the identity
+ * index scan. This is needed to filter lossy index matches, such as GiST
+ * multirange scans.
+ */
+static bool
+identity_key_equal(ChangeContext *chgcxt, TupleTableSlot *locator,
+ TupleTableSlot *candidate)
+{
+ Form_pg_index idx = chgcxt->cc_ident_index->rd_index;
+ AttrNumber last_key_attno = 0;
+
+ for (int i = 0; i < chgcxt->cc_ident_key_nentries; i++)
+ {
+ AttrNumber attno = idx->indkey.values[i];
+
+ Assert(attno > 0);
+ last_key_attno = Max(last_key_attno, attno);
+ }
+
+ slot_getsomeattrs(locator, last_key_attno);
+ slot_getsomeattrs(candidate, last_key_attno);
+
+ for (int i = 0; i < chgcxt->cc_ident_key_nentries; i++)
+ {
+ ScanKey entry = &chgcxt->cc_ident_key[i];
+ AttrNumber attno = idx->indkey.values[i];
+
+ Assert(attno > 0);
+
+ if (locator->tts_isnull[attno - 1] != candidate->tts_isnull[attno - 1])
+ return false;
+
+ if (locator->tts_isnull[attno - 1])
+ continue;
+
+ if (!DatumGetBool(FunctionCall2Coll(&entry->sk_func,
+ entry->sk_collation,
+ candidate->tts_values[attno - 1],
+ entry->sk_argument)))
+ return false;
+ }
+
+ return true;
+}
+
/*
* Decode and apply concurrent changes, up to (and including) the record whose
* LSN is 'end_of_wal'.
@@ -2944,13 +3004,20 @@ initialize_change_context(ChangeContext *chgcxt,
opcintype,
opno,
opcode;
+ StrategyNumber eq_strategy;
entry = &chgcxt->cc_ident_key[i];
opfamily = chgcxt->cc_ident_index->rd_opfamily[i];
opcintype = chgcxt->cc_ident_index->rd_opcintype[i];
+ eq_strategy = IndexAmTranslateCompareType(COMPARE_EQ,
+ chgcxt->cc_ident_index->rd_rel->relam,
+ opfamily, false);
+ if (eq_strategy == InvalidStrategy)
+ elog(ERROR, "failed to find equality strategy for index operator family %u for type %u",
+ opfamily, opcintype);
opno = get_opfamily_member(opfamily, opcintype, opcintype,
- BTEqualStrategyNumber);
+ eq_strategy);
if (!OidIsValid(opno))
elog(ERROR, "failed to find = operator for type %u", opcintype);
opcode = get_opcode(opno);
@@ -2960,7 +3027,7 @@ initialize_change_context(ChangeContext *chgcxt,
/* Initialize everything but argument. */
ScanKeyInit(entry,
i + 1,
- BTEqualStrategyNumber, opcode,
+ eq_strategy, opcode,
(Datum) 0);
entry->sk_collation = chgcxt->cc_ident_index->rd_indcollation[i];
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index f057d143d1a..c01d2fb095c 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -15,6 +15,8 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
inplace \
repack \
+ repack_temporal \
+ repack_temporal_multirange \
repack_toast \
syscache-update-pruned \
heap_lock_update
diff --git a/src/test/modules/injection_points/expected/repack_temporal.out b/src/test/modules/injection_points/expected/repack_temporal.out
new file mode 100644
index 00000000000..e6b06c00cec
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_temporal.out
@@ -0,0 +1,68 @@
+Parsed test spec with 2 sessions
+
+starting permutation: wait_before_lock update_target check_after_update wakeup_before_lock check_after_repack
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wait_before_lock:
+ REPACK (CONCURRENTLY) repack_temporal USING INDEX repack_temporal_pkey;
+ <waiting ...>
+step update_target:
+ UPDATE repack_temporal
+ SET label = 'updated'
+ WHERE id = '[2,3)' AND valid_at = '[2000-01-10,2000-01-20)';
+
+step check_after_update:
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode FROM pg_class WHERE relname = 'repack_temporal';
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal
+ ORDER BY id, valid_at, label;
+
+id |valid_at |label
+------+-----------------------+-------
+[1,10)|[01-01-2000,02-01-2000)|other
+[2,3) |[01-10-2000,01-20-2000)|updated
+(2 rows)
+
+step wakeup_before_lock:
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wait_before_lock: <... completed>
+step check_after_repack:
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode FROM pg_class WHERE relname = 'repack_temporal';
+
+ -- Expect 2, proving that repack has rewritten the table
+ SELECT count(DISTINCT node) FROM relfilenodes;
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal
+ ORDER BY id, valid_at, label;
+
+count
+-----
+ 2
+(1 row)
+
+id |valid_at |label
+------+-----------------------+-------
+[1,10)|[01-01-2000,02-01-2000)|other
+[2,3) |[01-10-2000,01-20-2000)|updated
+(2 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/expected/repack_temporal_multirange.out b/src/test/modules/injection_points/expected/repack_temporal_multirange.out
new file mode 100644
index 00000000000..3f5ff8cfb0f
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_temporal_multirange.out
@@ -0,0 +1,74 @@
+Parsed test spec with 2 sessions
+
+starting permutation: wait_before_lock update_target check_after_update wakeup_before_lock final_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wait_before_lock:
+ REPACK (CONCURRENTLY) repack_temporal_multirange
+ USING INDEX repack_temporal_multirange_pkey;
+ <waiting ...>
+step update_target:
+ UPDATE repack_temporal_multirange
+ SET label = 'updated'
+ WHERE id = int4multirange(int4range(1, 7))
+ AND valid_at = datemultirange(daterange('2000-01-01', '2000-02-01'));
+
+step check_after_update:
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode
+ FROM pg_class
+ WHERE relname = 'repack_temporal_multirange';
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal_multirange
+ ORDER BY id, valid_at, label;
+
+id |valid_at |label
+-------------+-------------------------+-------
+{[1,3),[5,7)}|{[01-01-2000,02-01-2000)}|other
+{[1,7)} |{[01-01-2000,02-01-2000)}|updated
+(2 rows)
+
+step wakeup_before_lock:
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wait_before_lock: <... completed>
+step final_check:
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode
+ FROM pg_class
+ WHERE relname = 'repack_temporal_multirange';
+
+ -- Expect 2, proving that repack has rewritten the table
+ SELECT count(DISTINCT node) FROM relfilenodes;
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal_multirange
+ ORDER BY id, valid_at, label;
+
+count
+-----
+ 2
+(1 row)
+
+id |valid_at |label
+-------------+-------------------------+-------
+{[1,3),[5,7)}|{[01-01-2000,02-01-2000)}|other
+{[1,7)} |{[01-01-2000,02-01-2000)}|updated
+(2 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index fb1418e2caa..59dba1cb023 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -46,6 +46,8 @@ tests += {
'basic',
'inplace',
'repack',
+ 'repack_temporal',
+ 'repack_temporal_multirange',
'repack_toast',
'syscache-update-pruned',
'heap_lock_update',
diff --git a/src/test/modules/injection_points/specs/repack_temporal.spec b/src/test/modules/injection_points/specs/repack_temporal.spec
new file mode 100644
index 00000000000..9629d502ec1
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_temporal.spec
@@ -0,0 +1,90 @@
+# REPACK (CONCURRENTLY) on a temporal replica identity index.
+#
+# The table's replica identity is a GiST index created by a temporal primary
+# key. A concurrent UPDATE changes a non-key column of one row, while another
+# row overlaps it on all indexed columns. Replay must still find the exact
+# target row.
+setup
+{
+ CREATE EXTENSION injection_points;
+
+ CREATE TABLE repack_temporal (
+ id int4range,
+ valid_at daterange,
+ label text,
+ PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
+ );
+
+ ALTER TABLE repack_temporal REPLICA IDENTITY USING INDEX repack_temporal_pkey;
+
+ INSERT INTO repack_temporal(id, valid_at, label)
+ VALUES
+ ('[1,10)', '[2000-01-01,2000-02-01)', 'other'),
+ ('[2,3)', '[2000-01-10,2000-01-20)', 'target');
+
+ CREATE TABLE relfilenodes(node oid);
+}
+
+teardown
+{
+ DROP TABLE repack_temporal;
+ DROP EXTENSION injection_points;
+ DROP TABLE relfilenodes;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
+}
+step wait_before_lock
+{
+ REPACK (CONCURRENTLY) repack_temporal USING INDEX repack_temporal_pkey;
+}
+step check_after_repack
+{
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode FROM pg_class WHERE relname = 'repack_temporal';
+
+ -- Expect 2, proving that repack has rewritten the table
+ SELECT count(DISTINCT node) FROM relfilenodes;
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal
+ ORDER BY id, valid_at, label;
+}
+teardown
+{
+ SELECT injection_points_detach('repack-concurrently-before-lock');
+}
+
+session s2
+step update_target
+{
+ UPDATE repack_temporal
+ SET label = 'updated'
+ WHERE id = '[2,3)' AND valid_at = '[2000-01-10,2000-01-20)';
+}
+step check_after_update
+{
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode FROM pg_class WHERE relname = 'repack_temporal';
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal
+ ORDER BY id, valid_at, label;
+}
+step wakeup_before_lock
+{
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+}
+
+permutation
+ wait_before_lock
+ update_target
+ check_after_update
+ wakeup_before_lock
+ check_after_repack
diff --git a/src/test/modules/injection_points/specs/repack_temporal_multirange.spec b/src/test/modules/injection_points/specs/repack_temporal_multirange.spec
new file mode 100644
index 00000000000..dfff1f2234d
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_temporal_multirange.spec
@@ -0,0 +1,102 @@
+# REPACK (CONCURRENTLY) on a temporal replica identity index with lossy
+# multirange equality.
+#
+# The leading identity column is an int4multirange. Two distinct rows have
+# different multirange values but the same union range, so GiST equality can
+# produce both as candidates and requires exact recheck.
+setup
+{
+ CREATE EXTENSION injection_points;
+
+ CREATE TABLE repack_temporal_multirange (
+ id int4multirange,
+ valid_at datemultirange,
+ label text,
+ PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
+ );
+
+ ALTER TABLE repack_temporal_multirange
+ REPLICA IDENTITY USING INDEX repack_temporal_multirange_pkey;
+
+ -- (1,3)+(5+7) is the same uninon range of (1-7), but needs recheck
+ INSERT INTO repack_temporal_multirange(id, valid_at, label)
+ VALUES
+ (int4multirange(int4range(1, 3), int4range(5, 7)),
+ datemultirange(daterange('2000-01-01', '2000-02-01')),
+ 'other'),
+ (int4multirange(int4range(1, 7)),
+ datemultirange(daterange('2000-01-01', '2000-02-01')),
+ 'target');
+
+ CREATE TABLE relfilenodes(node oid);
+}
+
+teardown
+{
+ DROP TABLE repack_temporal_multirange;
+ DROP EXTENSION injection_points;
+ DROP TABLE relfilenodes;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
+}
+step wait_before_lock
+{
+ REPACK (CONCURRENTLY) repack_temporal_multirange
+ USING INDEX repack_temporal_multirange_pkey;
+}
+step final_check
+{
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode
+ FROM pg_class
+ WHERE relname = 'repack_temporal_multirange';
+
+ -- Expect 2, proving that repack has rewritten the table
+ SELECT count(DISTINCT node) FROM relfilenodes;
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal_multirange
+ ORDER BY id, valid_at, label;
+}
+teardown
+{
+ SELECT injection_points_detach('repack-concurrently-before-lock');
+}
+
+session s2
+step update_target
+{
+ UPDATE repack_temporal_multirange
+ SET label = 'updated'
+ WHERE id = int4multirange(int4range(1, 7))
+ AND valid_at = datemultirange(daterange('2000-01-01', '2000-02-01'));
+}
+step check_after_update
+{
+ INSERT INTO relfilenodes(node)
+ SELECT relfilenode
+ FROM pg_class
+ WHERE relname = 'repack_temporal_multirange';
+
+ -- Expect 2 rows
+ SELECT id, valid_at, label
+ FROM repack_temporal_multirange
+ ORDER BY id, valid_at, label;
+}
+step wakeup_before_lock
+{
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+}
+
+permutation
+ wait_before_lock
+ update_target
+ check_after_update
+ wakeup_before_lock
+ final_check
--
2.50.1 (Apple Git-155)
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]
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