pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedTID Range Scans can return wrong results after scanning backwards
3+ messages / 3 participants
[nested] [flat]
Commit outlook (model estimate bayes-2.0, computed 2026-11-01; about the patch, not a person):
The activity on this thread indicates a low probability (1%) of a commit from this thread within 60 days.
Seek out Zsolt Parragi or Rafia Sabih or Steven Niu or Cary Huang or Junwang Zhao: they have history with this code.
What the estimate rests on: thread commits 2.079 (lowers it), author thread rate 0.32 (raises it), last msg days 3.68 (lowers it), msgs 30d 0.0 (lowers it)
Commits from this thread:
2026-09-23 374522aa63 ad230d5888 39a0db1011 25ebf0097f 6ceac2e125 cd7d3e129b c84dd31bd9 Fix incorrect block accounting in TID Range Scans
* TID Range Scans can return wrong results after scanning backwards
@ 2026-09-16 20:07 Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Davis @ 2026-09-16 20:07 UTC (permalink / raw)
To: pgsql-bugs
AI reported a bug (appended to this email) where TID Range Scans can
return the wrong results after changing the direction of the scan.
Regards,
Jeff Davis
SQL repro
---------
CREATE TABLE t (id int, data text) WITH (fillfactor = 10);
-- 5 rows per page, 20 pages: ctids (0,1) .. (19,5)
INSERT INTO t SELECT i, repeat('x', 100)
FROM generate_series(1, 100) i;
SET enable_seqscan = off;
BEGIN;
DECLARE c SCROLL CURSOR FOR
SELECT ctid FROM t WHERE ctid >= '(2,1)' AND ctid <= '(11,5)';
MOVE FORWARD 35 c; -- cursor is now on (8,5)
FETCH BACKWARD 2 c; -- (8,4), (8,3): correct
FETCH BACKWARD ALL c; -- should return 32 rows, returns 12
COMMIT;
Diagnosis
---------
heap_setscanlimits() stores the number of blocks to scan in
rs_numblocks. heapgettup_advance_block() then treats that field as a
remaining budget: it decrements it once for every block it hands out,
in either direction, and ends the scan when it reaches zero. That
model only holds for a scan that moves in one direction and consumes
exactly the blocks it requests. A change in direction breaks that.
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: TID Range Scans can return wrong results after scanning backwards
@ 2026-09-17 07:13 Ewan Young <kdbase.hack@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 3+ messages in thread
From: Ewan Young @ 2026-09-17 07:13 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs
Hi,
Thanks for the report!
I can reproduce this on master and on REL_19_STABLE (12 rows instead
of 32, as in your example), and the diagnosis matches what I see:
heapgettup_advance_block() spends rs_numblocks as a budget in both
directions, so a scan that reverses mid-way runs out early. It goes
back to when TID Range Scans made bidirectional limited scans possible
(v14); the budget logic itself is older, from the BRIN work, and the
2021 fix for the backward *start* block (16dfe253e31) didn't consider
direction changes.
The attached patch treats the limit as a fixed window of rs_numblocks
blocks starting at rs_startblock instead of a budget: the forward
direction ends when it reaches the block just past the window, and the
backward direction already ends at rs_startblock, since
heapgettup_initial_block() starts a backward scan at the window's last
block. rs_numblocks is no longer modified during the scan. TID Range
Scans are the only caller that scans a limited range in both
directions, so other heap_setscanlimits() users see no change.
Besides your example, I ran a randomized comparison of SCROLL cursor
sequences (random MOVE/FETCH FORWARD/BACKWARD/ALL/ABSOLUTE/RELATIVE
over random ctid ranges) between the TID Range Scan and a seqscan of
the same query: 135 of 1500 sequences differ on unpatched master, none
with the patch.
On Thu, Sep 17, 2026 at 4:07 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> AI reported a bug (appended to this email) where TID Range Scans can
> return the wrong results after changing the direction of the scan.
>
> Regards,
> Jeff Davis
>
>
>
> SQL repro
> ---------
>
> CREATE TABLE t (id int, data text) WITH (fillfactor = 10);
> -- 5 rows per page, 20 pages: ctids (0,1) .. (19,5)
> INSERT INTO t SELECT i, repeat('x', 100)
> FROM generate_series(1, 100) i;
> SET enable_seqscan = off;
> BEGIN;
> DECLARE c SCROLL CURSOR FOR
> SELECT ctid FROM t WHERE ctid >= '(2,1)' AND ctid <= '(11,5)';
> MOVE FORWARD 35 c; -- cursor is now on (8,5)
> FETCH BACKWARD 2 c; -- (8,4), (8,3): correct
> FETCH BACKWARD ALL c; -- should return 32 rows, returns 12
> COMMIT;
>
>
> Diagnosis
> ---------
>
> heap_setscanlimits() stores the number of blocks to scan in
> rs_numblocks. heapgettup_advance_block() then treats that field as a
> remaining budget: it decrements it once for every block it hands out,
> in either direction, and ends the scan when it reaches zero. That
> model only holds for a scan that moves in one direction and consumes
> exactly the blocks it requests. A change in direction breaks that.
>
>
>
>
>
--
Regards,
Ewan Young
Attachments:
v1-0001-Fix-TID-range-scans-that-change-direction-mid-scan.patch (application/octet-stream, 5.3K) download
From 032a5b44d2ceb931ad3303e3be3967ab2212d56c Mon Sep 17 00:00:00 2001
From: Ewan Young <kdbase.hack@gmail.com>
Date: Thu, 17 Sep 2026 22:58:12 +0800
Subject: [PATCH] Fix TID range scans that change direction mid-scan
heap_setscanlimits() stores the number of blocks to scan in
rs_numblocks, and heapgettup_advance_block() treated that as a
remaining budget, decrementing it once per block handed out in either
direction and ending the scan when it hit zero. That only works for
a scan that runs in one direction. A scrollable cursor over a TID
Range Scan that moves forward and then backward kept spending the
same budget, so the backward leg stopped early and rows in range were
never returned; a later forward leg was cut short as well.
Treat the limit as a fixed window of rs_numblocks blocks starting at
rs_startblock instead: a forward scan ends when it reaches the block
just past the window, and a backward scan ends at rs_startblock, which
the existing check already handled since heapgettup_initial_block()
starts a backward scan at the window's last block. rs_numblocks is no
longer modified during the scan.
TID Range Scans are the only caller that can scan a limited range in
both directions, so this only affects them.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/6b50233fcc0f7d6f4845f083777fa9702d9612e4.camel@j-davis.com
---
src/backend/access/heap/heapam.c | 28 +++++---
src/test/regress/expected/tidrangescan.out | 75 ++++++++++++++++++++++
src/test/regress/sql/tidrangescan.sql | 10 +++
3 files changed, 103 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 9ebb1b35d37..e35057e6cc0 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -908,10 +908,20 @@ heapgettup_advance_block(HeapScanDesc scan, BlockNumber block, ScanDirection dir
if (block == scan->rs_startblock)
return InvalidBlockNumber;
- /* check if the limit imposed by heap_setscanlimits() is met */
+ /*
+ * Check if the limit imposed by heap_setscanlimits() is met. The
+ * limit is a fixed window of rs_numblocks blocks starting at
+ * rs_startblock, so compare against its end rather than counting
+ * down a budget; a scan that changes direction (a SCROLL cursor)
+ * would otherwise exhaust the budget and stop early.
+ */
if (scan->rs_numblocks != InvalidBlockNumber)
{
- if (--scan->rs_numblocks == 0)
+ BlockNumber endblock;
+
+ endblock = (scan->rs_startblock + scan->rs_numblocks) %
+ scan->rs_nblocks;
+ if (block == endblock)
return InvalidBlockNumber;
}
@@ -919,17 +929,15 @@ heapgettup_advance_block(HeapScanDesc scan, BlockNumber block, ScanDirection dir
}
else
{
- /* we're done if the last block is the start position */
+ /*
+ * We're done if the last block is the start position. This also
+ * covers the limit imposed by heap_setscanlimits(), since the window
+ * begins at rs_startblock and heapgettup_initial_block() started us
+ * at its last block.
+ */
if (block == scan->rs_startblock)
return InvalidBlockNumber;
- /* check if the limit imposed by heap_setscanlimits() is met */
- if (scan->rs_numblocks != InvalidBlockNumber)
- {
- if (--scan->rs_numblocks == 0)
- return InvalidBlockNumber;
- }
-
/* wrap to the end of the heap when the last page was page 0 */
if (block == 0)
block = scan->rs_nblocks;
diff --git a/src/test/regress/expected/tidrangescan.out b/src/test/regress/expected/tidrangescan.out
index 89930f7bc7c..e567edfbff8 100644
--- a/src/test/regress/expected/tidrangescan.out
+++ b/src/test/regress/expected/tidrangescan.out
@@ -295,6 +295,81 @@ FETCH LAST c;
(0,10)
(1 row)
+COMMIT;
+-- a scrollable cursor that changes direction mid-scan must still stay within
+-- the block range computed by heap_setscanlimits()
+BEGIN;
+DECLARE c SCROLL CURSOR FOR SELECT ctid FROM tidrangescan WHERE ctid >= '(0,1)' AND ctid <= '(2,10)';
+MOVE FORWARD 25 c;
+FETCH BACKWARD 2 c;
+ ctid
+-------
+ (2,4)
+ (2,3)
+(2 rows)
+
+FETCH BACKWARD ALL c;
+ ctid
+--------
+ (2,2)
+ (2,1)
+ (1,10)
+ (1,9)
+ (1,8)
+ (1,7)
+ (1,6)
+ (1,5)
+ (1,4)
+ (1,3)
+ (1,2)
+ (1,1)
+ (0,10)
+ (0,9)
+ (0,8)
+ (0,7)
+ (0,6)
+ (0,5)
+ (0,4)
+ (0,3)
+ (0,2)
+ (0,1)
+(22 rows)
+
+FETCH FORWARD ALL c;
+ ctid
+--------
+ (0,1)
+ (0,2)
+ (0,3)
+ (0,4)
+ (0,5)
+ (0,6)
+ (0,7)
+ (0,8)
+ (0,9)
+ (0,10)
+ (1,1)
+ (1,2)
+ (1,3)
+ (1,4)
+ (1,5)
+ (1,6)
+ (1,7)
+ (1,8)
+ (1,9)
+ (1,10)
+ (2,1)
+ (2,2)
+ (2,3)
+ (2,4)
+ (2,5)
+ (2,6)
+ (2,7)
+ (2,8)
+ (2,9)
+ (2,10)
+(30 rows)
+
COMMIT;
DROP TABLE tidrangescan;
-- Tests for parallel TID Range Scans
diff --git a/src/test/regress/sql/tidrangescan.sql b/src/test/regress/sql/tidrangescan.sql
index 1ac3995e71c..55e266c431c 100644
--- a/src/test/regress/sql/tidrangescan.sql
+++ b/src/test/regress/sql/tidrangescan.sql
@@ -96,6 +96,16 @@ FETCH FIRST c;
FETCH LAST c;
COMMIT;
+-- a scrollable cursor that changes direction mid-scan must still stay within
+-- the block range computed by heap_setscanlimits()
+BEGIN;
+DECLARE c SCROLL CURSOR FOR SELECT ctid FROM tidrangescan WHERE ctid >= '(0,1)' AND ctid <= '(2,10)';
+MOVE FORWARD 25 c;
+FETCH BACKWARD 2 c;
+FETCH BACKWARD ALL c;
+FETCH FORWARD ALL c;
+COMMIT;
+
DROP TABLE tidrangescan;
-- Tests for parallel TID Range Scans
--
2.47.3
* Re: TID Range Scans can return wrong results after scanning backwards
@ 2026-09-23 13:34 David Rowley <dgrowleyml@gmail.com>
parent: Ewan Young <kdbase.hack@gmail.com>
0 siblings, 0 replies; 3+ messages in thread
From: David Rowley @ 2026-09-23 13:34 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-bugs
On Thu, 17 Sept 2026 at 19:14, Ewan Young <kdbase.hack@gmail.com> wrote:
> The attached patch treats the limit as a fixed window of rs_numblocks
> blocks starting at rs_startblock instead of a budget: the forward
> direction ends when it reaches the block just past the window, and the
> backward direction already ends at rs_startblock, since
> heapgettup_initial_block() starts a backward scan at the window's last
> block. rs_numblocks is no longer modified during the scan. TID Range
> Scans are the only caller that scans a limited range in both
> directions, so other heap_setscanlimits() users see no change.
I adjusted this and wrote the version for the backbranches (block
advancing is different pre-v16) and pushed it.
For the record, you really need to sense-check your LLM-generated
comments in the future. If patch authors don't check this sort of
stuff, then I really fail to see the point of posting these simple bug
fix patches. I really was unable to parse the "started us at its last
block" in the following:
+ * We're done if the last block is the start position. This also
+ * covers the limit imposed by heap_setscanlimits(), since the window
+ * begins at rs_startblock and heapgettup_initial_block() started us
+ * at its last block.
Anyway, I rewrote that into something that I could understand.
For the tests, I think they were far too verbose. I added some extra
MOVEs to make it so the expected results weren't as large.
David
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-09-23 13:34 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-16 20:07 TID Range Scans can return wrong results after scanning backwards Jeff Davis <pgsql@j-davis.com>
2026-09-17 07:13 ` Ewan Young <kdbase.hack@gmail.com>
2026-09-23 13:34 ` David Rowley <dgrowleyml@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox