Skip to content

Commit 5f6d5a1

Browse files
turebaPoroma-Banerjee
authored andcommitted
tsl/compression: fix stale sparse-index entries after rebuild_sparse_index
populate_sparse_index_columns() rewrites each compressed batch's sparse index metadata columns (min/max, first/last, bloom) in place via simple_table_tuple_update(), but discarded the TU_UpdateIndexes result that call computes instead of acting on it. When the rewrite wasn't HOT-eligible (typically because the row no longer fits on its original page), the btree index covering those columns never got a new entry for the row's new location, and the stale entry pointing at the superseded tuple was left behind. The stale entry usually goes unnoticed because PostgreSQL's HOT-chain "redirect" mechanism transparently follows it to the live tuple, as long as the chain stays on the same page. Once that chain is broken -- by ordinary heap pruning/vacuum, or whenever the rewrite must relocate to a different page -- an index or bitmap scan using the stale entry silently returns the wrong (or zero) rows for the affected batch, while a sequential scan still returns the correct data. This can surface as wrong query results for compressed hypertables after compress_index changes (e.g. via rebuild_sparse_index() or an ALTER TABLE compress_index change followed by recompression). Fix by opening the compressed table's indexes once per populate_sparse_index_columns() call and inserting the new entry whenever the update actually wrote a new tuple, mirroring the same open-index/simple-update/insert-index-entry sequence PostgreSQL's own CatalogTupleUpdate() uses for catalog tuples. This requires switching from simple_table_tuple_update() to simple_heap_update(): the former funnels the update through a TupleTableSlot and materializes a throwaway copy of the tuple inside heapam_tuple_update(), so the caller's own HeapTuple is never updated with the tuple's real post-update location or HOT status. simple_heap_update() operates directly on the passed HeapTuple, exactly as CatalogTupleUpdate() relies on -- consistent with the rest of this function, which already assumes a plain heap-backed relation (heap_modify_tuple, heap_deform_tuple). Adds a regression check that forces an index/bitmap scan after the rebuild_sparse_index() drop-and-restore dance in Test 10 and confirms per-device batch counts match a sequential scan; previously this would return fewer rows once the affected batch's index entry had gone stale.
1 parent 5bd0019 commit 5f6d5a1

4 files changed

Lines changed: 63 additions & 12 deletions

File tree

.unreleased/pr_10324

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fixes: #10324 Fix stale index entries after rebuild_sparse_index() on compressed chunks
2+
Thanks: @tureba for reporting and fixing stale sparse-index entries after rebuild

tsl/src/compression/recompress.c

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <postgres.h>
88
#include "debug_point.h"
99
#include <access/tableam.h>
10+
#include <catalog/indexing.h>
1011
#include <miscadmin.h>
1112
#include <parser/parse_coerce.h>
1213
#include <parser/parse_relation.h>
@@ -36,6 +37,7 @@
3637
#include "recompress.h"
3738
#include "sparse_index_bloom1.h"
3839
#include "ts_catalog/array_utils.h"
40+
#include "ts_catalog/catalog.h"
3941
#include "ts_catalog/chunk_column_stats.h"
4042
#include "ts_catalog/compression_chunk_size.h"
4143
#include "ts_catalog/compression_settings.h"
@@ -2561,7 +2563,7 @@ populate_sparse_index_columns(Relation compressed_rel, RowDecompressor *decompre
25612563
TupleDesc compressed_desc = RelationGetDescr(compressed_rel);
25622564
TableScanDesc scan = table_beginscan_compat(compressed_rel, GetActiveSnapshot(), 0, NULL, 0);
25632565
TupleTableSlot *scan_slot = table_slot_create(compressed_rel, NULL);
2564-
TupleTableSlot *update_slot = MakeSingleTupleTableSlot(compressed_desc, &TTSOpsHeapTuple);
2566+
CatalogIndexState indstate = CatalogOpenIndexes(compressed_rel);
25652567

25662568
while (table_scan_getnextslot(scan, ForwardScanDirection, scan_slot))
25672569
{
@@ -2601,20 +2603,22 @@ populate_sparse_index_columns(Relation compressed_rel, RowDecompressor *decompre
26012603
decompressor->compressed_datums,
26022604
decompressor->compressed_is_nulls,
26032605
repl);
2604-
ExecStoreHeapTuple(new_tuple, update_slot, false);
26052606

26062607
/*
2607-
* Sparse index metadata columns are not covered by any index.
2608-
* If indexes on metadata columns are added in the future,
2609-
* this will need to handle index updates via update_indexes.
2608+
* Sparse index metadata columns are covered by a btree index
2609+
* (segmentby, first/last time, and any minmax/bloom columns). If
2610+
* this update isn't HOT-eligible, the old index entries are left
2611+
* pointing at the superseded tuple, so we must insert new entries
2612+
* ourselves -- mirroring what CatalogTupleUpdate() does for
2613+
* catalog tuples. We use simple_heap_update() rather than
2614+
* simple_table_tuple_update() so that heap_update() writes the new
2615+
* tuple's location and HOT status directly into new_tuple, which
2616+
* ts_catalog_index_insert() (a no-op when the update was HOT)
2617+
* relies on below.
26102618
*/
26112619
TU_UpdateIndexes update_indexes;
2612-
simple_table_tuple_update(compressed_rel,
2613-
&tid,
2614-
update_slot,
2615-
GetActiveSnapshot(),
2616-
&update_indexes);
2617-
ExecClearTuple(update_slot);
2620+
simple_heap_update(compressed_rel, &tid, new_tuple, &update_indexes);
2621+
ts_catalog_index_insert(indstate, new_tuple);
26182622

26192623
/* Reset */
26202624
foreach_ptr(BatchMetadataBuilder, builder, builders)
@@ -2632,9 +2636,9 @@ populate_sparse_index_columns(Relation compressed_rel, RowDecompressor *decompre
26322636
}
26332637
}
26342638

2635-
ExecDropSingleTupleTableSlot(update_slot);
26362639
ExecDropSingleTupleTableSlot(scan_slot);
26372640
table_endscan(scan);
2641+
CatalogCloseIndexes(indstate);
26382642
}
26392643

26402644
void

tsl/test/expected/rebuild_sparse_index.out

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,35 @@ FROM :ic_schema1.:ic_table1 WHERE device = 'd2' LIMIT 1;
518518
-------+-------+-------
519519
f | f | f
520520

521+
-- Sparse index integrity: the (device, first_time, last_time) btree must be
522+
-- able to find every batch for every device after rebuild_sparse_index, not
523+
-- just the ones whose metadata rewrite happened to be a HOT update. Forced
524+
-- index/bitmap scans must return the same per-device batch counts as an
525+
-- unindexed scan.
526+
SET enable_indexscan = off;
527+
SET enable_bitmapscan = off;
528+
SELECT device, count(*) FROM :ic_schema1.:ic_table1 GROUP BY device ORDER BY device;
529+
device | count
530+
--------+-------
531+
d1 | 2
532+
d2 | 2
533+
d3 | 2
534+
d4 | 2
535+
d5 | 2
536+
537+
RESET enable_indexscan;
538+
RESET enable_bitmapscan;
539+
SET enable_seqscan = off;
540+
SELECT device, count(*) FROM :ic_schema1.:ic_table1 GROUP BY device ORDER BY device;
541+
device | count
542+
--------+-------
543+
d1 | 2
544+
d2 | 2
545+
d3 | 2
546+
d4 | 2
547+
d5 | 2
548+
549+
RESET enable_seqscan;
521550
SELECT decompress_chunk(:'ichunk1');
522551
decompress_chunk
523552
----------------------------------------
@@ -594,6 +623,7 @@ SELECT * FROM rsi_integrity WHERE device = 'd1' AND humidity = 42 AND label = 'l
594623
-> Index Scan using _hyper_2_3_chunk_compressed_device__ts_meta_v2_first_time___idx on _hyper_2_3_chunk_compressed (actual rows=0.00 loops=1)
595624
Index Cond: (device = 'd1'::text)
596625
Filter: _timescaledb_functions.bloom1_contains_any_hashes(regress-test-bloom_577c_humidity_label, TEST-HASHES::bigint[])
626+
Rows Removed by Filter: 2
597627
-> Seq Scan on _hyper_2_4_chunk (actual rows=0.00 loops=1)
598628
Filter: ((device = 'd1'::text) AND (humidity = 42) AND (label = 'label_0'::text))
599629
Rows Removed by Filter: 8640

tsl/test/sql/rebuild_sparse_index.sql

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,21 @@ SELECT _timescaledb_functions.bloom1_contains(:"bloom_label", 'nonexistent'::tex
355355
_timescaledb_functions.bloom1_contains(:"bloom_label", 'NOPE'::text) AS nope3
356356
FROM :ic_schema1.:ic_table1 WHERE device = 'd2' LIMIT 1;
357357

358+
-- Sparse index integrity: the (device, first_time, last_time) btree must be
359+
-- able to find every batch for every device after rebuild_sparse_index, not
360+
-- just the ones whose metadata rewrite happened to be a HOT update. Forced
361+
-- index/bitmap scans must return the same per-device batch counts as an
362+
-- unindexed scan.
363+
SET enable_indexscan = off;
364+
SET enable_bitmapscan = off;
365+
SELECT device, count(*) FROM :ic_schema1.:ic_table1 GROUP BY device ORDER BY device;
366+
RESET enable_indexscan;
367+
RESET enable_bitmapscan;
368+
369+
SET enable_seqscan = off;
370+
SELECT device, count(*) FROM :ic_schema1.:ic_table1 GROUP BY device ORDER BY device;
371+
RESET enable_seqscan;
372+
358373
SELECT decompress_chunk(:'ichunk1');
359374

360375
-- Test 11: composite bloom integrity (drop, rebuild, verify via explain)

0 commit comments

Comments
 (0)