Can rs_cindex be < 0 for bitmap heap scans?
Hi,
HeapScanDescData->rs_cindex (the current index into the array of
visible tuples stored in the heap scan descriptor) is used for
multiple scan types, but for bitmap heap scans, AFAICT, it should
never be < 0.
As such, I find this test in heapam_scan_bitmap_next_tuple() pretty confusing.
if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
I know it seems innocuous, but I find it pretty distracting. Am I
missing something? Could it somehow be < 0?
If not, I propose removing that part of the if statement like in the
attached patch.
- Melanie
Attachments:
v1-0001-Remove-unused-rs_cindex-condition-in-heap-AM-bitm.patchapplication/octet-stream; name=v1-0001-Remove-unused-rs_cindex-condition-in-heap-AM-bitm.patchDownload
From 1d6b8ec42d27a7223cc5221a35370607f9e29329 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Thu, 17 Oct 2024 15:51:24 -0400
Subject: [PATCH v1] Remove unused rs_cindex condition in heap AM bitmap
callback
HeapScanDescData.rs_cindex can't be less than 0 for bitmap heap scans.
Remove this test from heapam_scan_bitmap_next_tuple() and add an
assertion to ensure this stays true.
Also initialize HeapScanDescData.rs_cindex to 0 in initscan().
Author: Melanie Plageman
---
src/backend/access/heap/heapam.c | 1 +
src/backend/access/heap/heapam_handler.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index da5e656a08d..cbc5f62aff6 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -378,6 +378,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_cindex = 0;
/*
* Initialize to ForwardScanDirection because it is most common and
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8c59b77b64f..bdb7599684b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2269,7 +2269,8 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
/*
* Out of range? If so, nothing more to look at on this page
*/
- if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
+ Assert(hscan->rs_cindex >= 0);
+ if (hscan->rs_cindex >= hscan->rs_ntuples)
return false;
targoffset = hscan->rs_vistuples[hscan->rs_cindex];
--
2.45.2
On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman <melanieplageman@gmail.com>
wrote:
Hi,
HeapScanDescData->rs_cindex (the current index into the array of
visible tuples stored in the heap scan descriptor) is used for
multiple scan types, but for bitmap heap scans, AFAICT, it should
never be < 0.As such, I find this test in heapam_scan_bitmap_next_tuple() pretty
confusing.if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
I know it seems innocuous, but I find it pretty distracting. Am I
missing something? Could it somehow be < 0?If not, I propose removing that part of the if statement like in the
attached patch.
You are right it can never be < 0 in this case at least. In fact you don't
need to explicitly set it to 0 in initscan[1]@@ -378,6 +378,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) ItemPointerSetInvalid(&scan->rs_ctup.t_self); scan->rs_cbuf = InvalidBuffer; scan->rs_cblock = InvalidBlockNumber; + scan->rs_cindex = 0;, because before
calling heapam_scan_bitmap_next_tuple() we must
call heapam_scan_bitmap_next_block() and this function is initializing this
to 0 (hscan->rs_cindex = 0;). Anyway no object even if you prefer to
initialize in initscan(), just the point is that we are already doing it
for each block before fetching tuples from the block.
[1]: @@ -378,6 +378,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) ItemPointerSetInvalid(&scan->rs_ctup.t_self); scan->rs_cbuf = InvalidBuffer; scan->rs_cblock = InvalidBlockNumber; + scan->rs_cindex = 0;
@@ -378,6 +378,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool
keep_startblock)
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_cindex = 0;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Thanks for the reply, Dilip!
On Thu, Oct 24, 2024 at 12:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman <melanieplageman@gmail.com> wrote:
HeapScanDescData->rs_cindex (the current index into the array of
visible tuples stored in the heap scan descriptor) is used for
multiple scan types, but for bitmap heap scans, AFAICT, it should
never be < 0.You are right it can never be < 0 in this case at least.
Cool, thanks for confirming. I will commit this change then.
In fact you don't need to explicitly set it to 0 in initscan[1], because before calling heapam_scan_bitmap_next_tuple() we must call heapam_scan_bitmap_next_block() and this function is initializing this to 0 (hscan->rs_cindex = 0;). Anyway no object even if you prefer to initialize in initscan(), just the point is that we are already doing it for each block before fetching tuples from the block.
I am inclined to still initialize it to 0 in initscan(). I was
refactoring the bitmap heap scan code to use the read stream API and
after moving some things around, this field was no longer getting
initialized in heapam_scan_bitmap_next_block(). While that may not be
a good reason to change it in this patch, most of the other fields in
the heap scan descriptor (like rs_cbuf and rs_cblock) are
re-initialized in initscan(), so it seems okay to do that there even
though it isn't strictly necessary on master.
- Melanie
On Thu, Oct 24, 2024 at 7:11 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:
Thanks for the reply, Dilip!
On Thu, Oct 24, 2024 at 12:50 AM Dilip Kumar <dilipbalaut@gmail.com>
wrote:On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman <
melanieplageman@gmail.com> wrote:
HeapScanDescData->rs_cindex (the current index into the array of
visible tuples stored in the heap scan descriptor) is used for
multiple scan types, but for bitmap heap scans, AFAICT, it should
never be < 0.You are right it can never be < 0 in this case at least.
Cool, thanks for confirming. I will commit this change then.
+1
In fact you don't need to explicitly set it to 0 in initscan[1], because
before calling heapam_scan_bitmap_next_tuple() we must call
heapam_scan_bitmap_next_block() and this function is initializing this to 0
(hscan->rs_cindex = 0;). Anyway no object even if you prefer to initialize
in initscan(), just the point is that we are already doing it for each
block before fetching tuples from the block.I am inclined to still initialize it to 0 in initscan(). I was
refactoring the bitmap heap scan code to use the read stream API and
after moving some things around, this field was no longer getting
initialized in heapam_scan_bitmap_next_block(). While that may not be
a good reason to change it in this patch, most of the other fields in
the heap scan descriptor (like rs_cbuf and rs_cblock) are
re-initialized in initscan(), so it seems okay to do that there even
though it isn't strictly necessary on master.
make sense
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Thu, Oct 24, 2024 at 9:40 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Thu, Oct 24, 2024 at 12:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman <melanieplageman@gmail.com> wrote:
HeapScanDescData->rs_cindex (the current index into the array of
visible tuples stored in the heap scan descriptor) is used for
multiple scan types, but for bitmap heap scans, AFAICT, it should
never be < 0.You are right it can never be < 0 in this case at least.
Cool, thanks for confirming. I will commit this change then.
Tom suggested off-list that if rs_cindex can't be zero, then it should
be unsigned. I checked the other scan types using the
HeapScanDescData, and it seems none of them use values of rs_cindex or
rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
rs_cindex unsigned.
- Melanie
Attachments:
v2-0001-Make-rs_cindex-and-rs_ntuples-unsigned.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Make-rs_cindex-and-rs_ntuples-unsigned.patchDownload
From b770f675c8130bfa2d1a25fbfc8064f1dd7cbbe6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 25 Oct 2024 09:09:24 -0400
Subject: [PATCH v2] Make rs_cindex and rs_ntuples unsigned
HeapScanDescData.rs_cindex and rs_ntuples can't be less than 0. All scan
types using the heap scan descriptor expect these values to be >= 0.
Make that expectation clear by making rs_cindex and rs_ntuples unsigned.
Also remove the test in heapam_scan_bitmap_next_tuple() that checks if
rs_cindex < 0. This was never true, but now that rs_cindex is unsigned,
it makes even less sense.
While we are at it, initialize both rs_cindex and rs_ntuples to 0 in
initscan().
Author: Melanie Plageman
---
src/backend/access/heap/heapam.c | 7 +++++--
src/backend/access/heap/heapam_handler.c | 2 +-
src/include/access/heapam.h | 4 ++--
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4c8febdc811..d127b75a1a5 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -378,6 +378,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_ntuples = 0;
+ scan->rs_cindex = 0;
/*
* Initialize to ForwardScanDirection because it is most common and
@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
HeapTuple tuple = &(scan->rs_ctup);
Page page;
- int lineindex;
- int linesleft;
+ uint32 lineindex;
+ uint32 linesleft;
if (likely(scan->rs_inited))
{
@@ -989,6 +991,7 @@ continue_page:
ItemId lpp;
OffsetNumber lineoff;
+ Assert(lineindex >= 0 && lineindex <= scan->rs_ntuples);
lineoff = scan->rs_vistuples[lineindex];
lpp = PageGetItemId(page, lineoff);
Assert(ItemIdIsNormal(lpp));
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8c59b77b64f..82668fab19a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2269,7 +2269,7 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
/*
* Out of range? If so, nothing more to look at on this page
*/
- if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
+ if (hscan->rs_cindex >= hscan->rs_ntuples)
return false;
targoffset = hscan->rs_vistuples[hscan->rs_cindex];
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index b951466ced2..21c7b70edf2 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,8 +103,8 @@ typedef struct HeapScanDescData
int rs_empty_tuples_pending;
/* these fields only used in page-at-a-time mode and for bitmap scans */
- int rs_cindex; /* current tuple's index in vistuples */
- int rs_ntuples; /* number of visible tuples on page */
+ uint32 rs_cindex; /* current tuple's index in vistuples */
+ uint32 rs_ntuples; /* number of visible tuples on page */
OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their offsets */
} HeapScanDescData;
typedef struct HeapScanDescData *HeapScanDesc;
--
2.34.1
On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
Tom suggested off-list that if rs_cindex can't be zero, then it should
be unsigned. I checked the other scan types using the
HeapScanDescData, and it seems none of them use values of rs_cindex or
rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
rs_cindex unsigned.
I realized one of the asserts was superfluous. Please find v3 attached.
- Melanie
Attachments:
v3-0001-Make-rs_cindex-and-rs_ntuples-unsigned.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Make-rs_cindex-and-rs_ntuples-unsigned.patchDownload
From 262984fde17c1ae3ba220eb5533c8598fb55cae1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 25 Oct 2024 09:09:24 -0400
Subject: [PATCH v3] Make rs_cindex and rs_ntuples unsigned
HeapScanDescData.rs_cindex and rs_ntuples can't be less than 0. All scan
types using the heap scan descriptor expect these values to be >= 0.
Make that expectation clear by making rs_cindex and rs_ntuples unsigned.
Also remove the test in heapam_scan_bitmap_next_tuple() that checks if
rs_cindex < 0. This was never true, but now that rs_cindex is unsigned,
it makes even less sense.
While we are at it, initialize both rs_cindex and rs_ntuples to 0 in
initscan().
Author: Melanie Plageman
---
src/backend/access/heap/heapam.c | 7 +++++--
src/backend/access/heap/heapam_handler.c | 2 +-
src/include/access/heapam.h | 4 ++--
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4c8febdc811..194f64c540d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -378,6 +378,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_ntuples = 0;
+ scan->rs_cindex = 0;
/*
* Initialize to ForwardScanDirection because it is most common and
@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
HeapTuple tuple = &(scan->rs_ctup);
Page page;
- int lineindex;
- int linesleft;
+ uint32 lineindex;
+ uint32 linesleft;
if (likely(scan->rs_inited))
{
@@ -989,6 +991,7 @@ continue_page:
ItemId lpp;
OffsetNumber lineoff;
+ Assert(lineindex <= scan->rs_ntuples);
lineoff = scan->rs_vistuples[lineindex];
lpp = PageGetItemId(page, lineoff);
Assert(ItemIdIsNormal(lpp));
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 8c59b77b64f..82668fab19a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2269,7 +2269,7 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
/*
* Out of range? If so, nothing more to look at on this page
*/
- if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)
+ if (hscan->rs_cindex >= hscan->rs_ntuples)
return false;
targoffset = hscan->rs_vistuples[hscan->rs_cindex];
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index b951466ced2..21c7b70edf2 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,8 +103,8 @@ typedef struct HeapScanDescData
int rs_empty_tuples_pending;
/* these fields only used in page-at-a-time mode and for bitmap scans */
- int rs_cindex; /* current tuple's index in vistuples */
- int rs_ntuples; /* number of visible tuples on page */
+ uint32 rs_cindex; /* current tuple's index in vistuples */
+ uint32 rs_ntuples; /* number of visible tuples on page */
OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their offsets */
} HeapScanDescData;
typedef struct HeapScanDescData *HeapScanDesc;
--
2.34.1
On Fri, Oct 25, 2024 at 10:07 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:
On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:Tom suggested off-list that if rs_cindex can't be zero, then it should
be unsigned. I checked the other scan types using the
HeapScanDescData, and it seems none of them use values of rs_cindex or
rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
rs_cindex unsigned.
@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
HeapTuple tuple = &(scan->rs_ctup);
Page page;
- int lineindex;
- int linesleft;
+ uint32 lineindex;
+ uint32 linesleft;IMHO we can't make "lineindex" as uint32, because just see the first code
block[1]if (likely(scan->rs_inited)) { /* continue from previously returned page/tuple */ page = BufferGetPage(scan->rs_cbuf); of heapgettup_pagemode(), we use this index as +ve (Forward scan
)as well as -ve (Backward scan).
[1]: if (likely(scan->rs_inited)) { /* continue from previously returned page/tuple */ page = BufferGetPage(scan->rs_cbuf);
if (likely(scan->rs_inited))
{
/* continue from previously returned page/tuple */
page = BufferGetPage(scan->rs_cbuf);
lineindex = scan->rs_cindex + dir;
if (ScanDirectionIsForward(dir))
--Refer definition of ScanDirection
typedef enum ScanDirection
{
BackwardScanDirection = -1,
NoMovementScanDirection = 0,
ForwardScanDirection = 1
} ScanDirection;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Sat, Oct 26, 2024 at 12:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Fri, Oct 25, 2024 at 10:07 PM Melanie Plageman <melanieplageman@gmail.com> wrote:
On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:Tom suggested off-list that if rs_cindex can't be zero, then it should
be unsigned. I checked the other scan types using the
HeapScanDescData, and it seems none of them use values of rs_cindex or
rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
rs_cindex unsigned.@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan, { HeapTuple tuple = &(scan->rs_ctup); Page page; - int lineindex; - int linesleft; + uint32 lineindex; + uint32 linesleft;IMHO we can't make "lineindex" as uint32, because just see the first code block[1] of heapgettup_pagemode(), we use this index as +ve (Forward scan )as well as -ve (Backward scan).
[1]
if (likely(scan->rs_inited))
{
/* continue from previously returned page/tuple */
page = BufferGetPage(scan->rs_cbuf);lineindex = scan->rs_cindex + dir;
if (ScanDirectionIsForward(dir))--Refer definition of ScanDirection
typedef enum ScanDirection
{
BackwardScanDirection = -1,
NoMovementScanDirection = 0,
ForwardScanDirection = 1
} ScanDirection;
Yes, so in the case of backwards scan, if scan->rs_cindex is 0, when
dir is -1, lineindex will wrap around, but we don't use it in that
case because linesleft will be 0 and then we will not meet the
conditions to execute the code in the loop under continue_page. And in
the loop, when adding -1 to lineindex, for backwards scan, it always
starts at linesleft and ticks down to 0.
We could add an if statement above the goto that says something like
if (linesleft > 0)
goto continue_page;
Would that make it clearer?
- Melanie
On Sat, Oct 26, 2024 at 5:30 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:
On Sat, Oct 26, 2024 at 12:17 AM Dilip Kumar <dilipbalaut@gmail.com>
wrote:On Fri, Oct 25, 2024 at 10:07 PM Melanie Plageman <
melanieplageman@gmail.com> wrote:
On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:Tom suggested off-list that if rs_cindex can't be zero, then it should
be unsigned. I checked the other scan types using the
HeapScanDescData, and it seems none of them use values of rs_cindex or
rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
rs_cindex unsigned.@@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan, { HeapTuple tuple = &(scan->rs_ctup); Page page; - int lineindex; - int linesleft; + uint32 lineindex; + uint32 linesleft;IMHO we can't make "lineindex" as uint32, because just see the first
code block[1] of heapgettup_pagemode(), we use this index as +ve (Forward
scan )as well as -ve (Backward scan).Yes, so in the case of backwards scan, if scan->rs_cindex is 0, when
dir is -1, lineindex will wrap around, but we don't use it in that
case because linesleft will be 0 and then we will not meet the
conditions to execute the code in the loop under continue_page. And in
the loop, when adding -1 to lineindex, for backwards scan, it always
starts at linesleft and ticks down to 0.
Yeah you are right, although the lineindex will wrap around when rs_cindex
is 0 , it would not be used. So, it won’t actually cause any issues, but
I’m not comfortable with the unintentional wraparound. I would have left
"scan->rs_cindex" as int itself but I am fine with whatever you prefer.
We could add an if statement above the goto that says something like
if (linesleft > 0)
goto continue_page;Would that make it clearer?
Not sure it would make it clearer. In fact, In common cases it would add
an extra instruction to check the if (linesleft > 0).
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Mon, Oct 28, 2024 at 2:27 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Oct 26, 2024 at 5:30 PM Melanie Plageman <melanieplageman@gmail.com> wrote:
Yes, so in the case of backwards scan, if scan->rs_cindex is 0, when
dir is -1, lineindex will wrap around, but we don't use it in that
case because linesleft will be 0 and then we will not meet the
conditions to execute the code in the loop under continue_page. And in
the loop, when adding -1 to lineindex, for backwards scan, it always
starts at linesleft and ticks down to 0.Yeah you are right, although the lineindex will wrap around when rs_cindex is 0 , it would not be used. So, it won’t actually cause any issues, but I’m not comfortable with the unintentional wraparound. I would have left "scan->rs_cindex" as int itself but I am fine with whatever you prefer.
So, I don't see -1 as different from the wrapped around value with an
unsigned data type. Also neither is a valid number of entries in
rs_vistuples (which is limited to MaxHeapTuplesPerPage).
That being said, I can see how having lineindex be an invalid value
could be confusing -- either with an unsigned or signed data type for
rs_cindex. I think to make this clear we would have to add another
special case for backwards and no movement scan.
For now, I've committed the version of the patch I proposed above (v3).
We could add an if statement above the goto that says something like
if (linesleft > 0)
goto continue_page;Would that make it clearer?
Not sure it would make it clearer. In fact, In common cases it would add an extra instruction to check the if (linesleft > 0).
Yes, indeed.
Thank you for taking a look and being so responsive on this thread.
- Melanie
Hi.
Em qua., 18 de dez. de 2024 às 14:01, Melanie Plageman <
melanieplageman@gmail.com> escreveu:
On Mon, Oct 28, 2024 at 2:27 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Oct 26, 2024 at 5:30 PM Melanie Plageman <
melanieplageman@gmail.com> wrote:
Yes, so in the case of backwards scan, if scan->rs_cindex is 0, when
dir is -1, lineindex will wrap around, but we don't use it in that
case because linesleft will be 0 and then we will not meet the
conditions to execute the code in the loop under continue_page. And in
the loop, when adding -1 to lineindex, for backwards scan, it always
starts at linesleft and ticks down to 0.Yeah you are right, although the lineindex will wrap around when
rs_cindex is 0 , it would not be used. So, it won’t actually cause any
issues, but I’m not comfortable with the unintentional wraparound. I would
have left "scan->rs_cindex" as int itself but I am fine with whatever you
prefer.So, I don't see -1 as different from the wrapped around value with an
unsigned data type. Also neither is a valid number of entries in
rs_vistuples (which is limited to MaxHeapTuplesPerPage).That being said, I can see how having lineindex be an invalid value
could be confusing -- either with an unsigned or signed data type for
rs_cindex. I think to make this clear we would have to add another
special case for backwards and no movement scan.For now, I've committed the version of the patch I proposed above (v3).
What happens if *rs_tuples* equal to zero in function
*SampleHeapTupleVisible*?
end = hscan->rs_ntuples - 1;
Would be good to fix the binary search too, now that unsigned types are
used.
Patch attached.
best regards,
Ranier Vilela
Attachments:
fix-possible-overflow.patchapplication/octet-stream; name=fix-possible-overflow.patchDownload
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index d0e5922eed..fa18f69eb4 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2575,7 +2575,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
{
HeapScanDesc hscan = (HeapScanDesc) scan;
- if (scan->rs_flags & SO_ALLOW_PAGEMODE)
+ if (scan->rs_flags & SO_ALLOW_PAGEMODE && hscan->rs_ntuples != 0)
{
/*
* In pageatatime mode, heap_prepare_pagescan() already did visibility
@@ -2586,12 +2586,12 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- int start = 0,
+ uint32 start = 0,
end = hscan->rs_ntuples - 1;
while (start <= end)
{
- int mid = (start + end) / 2;
+ uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];
if (tupoffset == curoffset)
On Wed, Dec 18, 2024 at 1:23 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi.
Em qua., 18 de dez. de 2024 às 14:01, Melanie Plageman <melanieplageman@gmail.com> escreveu:
For now, I've committed the version of the patch I proposed above (v3).
What happens if *rs_tuples* equal to zero in function *SampleHeapTupleVisible*?
end = hscan->rs_ntuples - 1;
Ah yes, it seems like just changing the top if statement to
if (scan->rs_flags & SO_ALLOW_PAGEMODE &&
hscan->rs_ntuples > 0)
Thanks for identifying this.
Would be good to fix the binary search too, now that unsigned types are used.
You just mean the one in SampleHeapTupleVisible(), right?
Patch attached.
I'm not sure the attached patch is quite right because if rs_ntuples
is 0, it should still enter the first if statement and then return
false. In fact, with your patch, I think we would incorrectly not
return a value when rs_ntuples is 0 from SampleHeapTupleVisible().
How about one of these options:
option 1:
most straightforward fix
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index d0e5922eed7..fa03bedd4b8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2577,6 +2577,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, if (scan->rs_flags & SO_ALLOW_PAGEMODE)
{
+ int start, end;
+
+ if (hscan->rs_ntuples == 0)
+ return false;
+
/*
* In pageatatime mode, heap_prepare_pagescan() already did visibility
* checks, so just look at the info it left in rs_vistuples[].
@@ -2586,8 +2591,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- int start = 0,
- end = hscan->rs_ntuples - 1;
+ start = 0;
+ end = hscan->rs_ntuples - 1; while (start <= end)
{
option 2:
change the binary search code a bit more but avoid the extra branch
introduced by option 1.
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index d0e5922eed7..c8e25bdd197 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2586,18 +2586,17 @@ SampleHeapTupleVisible(TableScanDesc scan,
Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- int start = 0,
- end = hscan->rs_ntuples - 1;
+ uint32 start = 0, end = hscan->rs_ntuples;- while (start <= end)
+ while (start < end)
{
- int mid = (start + end) / 2;
+ int mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid]; if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}
- Melanie
On Wed, Dec 18, 2024 at 3:13 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
option 1:
most straightforward fixdiff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index d0e5922eed7..fa03bedd4b8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2577,6 +2577,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,if (scan->rs_flags & SO_ALLOW_PAGEMODE) { + int start, end; + + if (hscan->rs_ntuples == 0) + return false; + /* * In pageatatime mode, heap_prepare_pagescan() already did visibility * checks, so just look at the info it left in rs_vistuples[]. @@ -2586,8 +2591,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, * in increasing order, but it's not clear that there would be enough * gain to justify the restriction. */ - int start = 0, - end = hscan->rs_ntuples - 1; + start = 0; + end = hscan->rs_ntuples - 1;while (start <= end)
{option 2:
change the binary search code a bit more but avoid the extra branch
introduced by option 1.diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index d0e5922eed7..c8e25bdd197 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2586,18 +2586,17 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, * in increasing order, but it's not clear that there would be enough * gain to justify the restriction. */ - int start = 0, - end = hscan->rs_ntuples - 1; + uint32 start = 0, end = hscan->rs_ntuples;- while (start <= end) + while (start < end) { - int mid = (start + end) / 2; + int mid = (start + end) / 2; OffsetNumber curoffset = hscan->rs_vistuples[mid];if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}
I pushed the straightforward option for now so that it's fixed.
- Melanie
On Thu, Dec 19, 2024 at 8:18 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
I pushed the straightforward option for now so that it's fixed.
I think this binary search code now has a risk of underflow. If 'mid'
is calculated as zero, the second 'if' branch will cause 'end' to
underflow.
Maybe we need to do something like below.
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2600,7 +2600,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
+ {
+ if (mid == 0)
+ return false;
end = mid - 1;
+ }
else
start = mid + 1;
}Alternatively, we can revert 'start' and 'end' to signed int as they
were before.
Thanks
Richard
Hi.
Sorry for not responding quickly.
I have been without communication until now.
Em qua., 18 de dez. de 2024 às 17:13, Melanie Plageman <
melanieplageman@gmail.com> escreveu:
On Wed, Dec 18, 2024 at 1:23 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi.
Em qua., 18 de dez. de 2024 às 14:01, Melanie Plageman <
melanieplageman@gmail.com> escreveu:
For now, I've committed the version of the patch I proposed above (v3).
What happens if *rs_tuples* equal to zero in function
*SampleHeapTupleVisible*?
end = hscan->rs_ntuples - 1;
Ah yes, it seems like just changing the top if statement to
if (scan->rs_flags & SO_ALLOW_PAGEMODE &&
hscan->rs_ntuples > 0)Thanks for identifying this.
Would be good to fix the binary search too, now that unsigned types are
used.
You just mean the one in SampleHeapTupleVisible(), right?
Patch attached.
I'm not sure the attached patch is quite right because if rs_ntuples
is 0, it should still enter the first if statement and then return
false. In fact, with your patch, I think we would incorrectly not
return a value when rs_ntuples is 0 from SampleHeapTupleVisible().
I'm wondering if *rs_tuples* is zero, would be run the second search
*HeapTupleSatisfiesVisibility*.
How about one of these options:
option 1:
most straightforward fixdiff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index d0e5922eed7..fa03bedd4b8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2577,6 +2577,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,if (scan->rs_flags & SO_ALLOW_PAGEMODE) { + int start, end; + + if (hscan->rs_ntuples == 0) + return false; + /* * In pageatatime mode, heap_prepare_pagescan() already did visibility * checks, so just look at the info it left in rs_vistuples[]. @@ -2586,8 +2591,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, * in increasing order, but it's not clear that there would be enough * gain to justify the restriction. */ - int start = 0, - end = hscan->rs_ntuples - 1; + start = 0; + end = hscan->rs_ntuples - 1;while (start <= end)
{option 2:
change the binary search code a bit more but avoid the extra branch
introduced by option 1.diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index d0e5922eed7..c8e25bdd197 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2586,18 +2586,17 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, * in increasing order, but it's not clear that there would be enough * gain to justify the restriction. */ - int start = 0, - end = hscan->rs_ntuples - 1; + uint32 start = 0, end = hscan->rs_ntuples;- while (start <= end) + while (start < end) { - int mid = (start + end) / 2; + int mid = (start + end) / 2; OffsetNumber curoffset = hscan->rs_vistuples[mid];if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}
I'm looking at the commit and the replies in the thread.
best regards,
Ranier Vilela
Em qua., 18 de dez. de 2024 às 23:50, Richard Guo <guofenglinux@gmail.com>
escreveu:
On Thu, Dec 19, 2024 at 8:18 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:I pushed the straightforward option for now so that it's fixed.
I think this binary search code now has a risk of underflow. If 'mid'
is calculated as zero, the second 'if' branch will cause 'end' to
underflow.Maybe we need to do something like below.
--- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2600,7 +2600,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, if (tupoffset == curoffset) return true; else if (tupoffset < curoffset) + { + if (mid == 0) + return false; end = mid - 1; + } else start = mid + 1; }Alternatively, we can revert 'start' and 'end' to signed int as they
were before.
How would it be *signed*?
Wouldn't overflow happen in this case?
rs_tuples now can be
UINT_MAX = 4294967295
best regards,
Ranier Vilela
Em qua., 18 de dez. de 2024 às 20:18, Melanie Plageman <
melanieplageman@gmail.com> escreveu:
On Wed, Dec 18, 2024 at 3:13 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:option 1:
most straightforward fixdiff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index d0e5922eed7..fa03bedd4b8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2577,6 +2577,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Bufferbuffer,
if (scan->rs_flags & SO_ALLOW_PAGEMODE) { + int start, end; + + if (hscan->rs_ntuples == 0) + return false; + /* * In pageatatime mode, heap_prepare_pagescan() already didvisibility
* checks, so just look at the info it left in rs_vistuples[].
@@ -2586,8 +2591,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Bufferbuffer,
* in increasing order, but it's not clear that there would be
enough
* gain to justify the restriction. */ - int start = 0, - end = hscan->rs_ntuples - 1; + start = 0; + end = hscan->rs_ntuples - 1;while (start <= end)
{option 2:
change the binary search code a bit more but avoid the extra branch
introduced by option 1.diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index d0e5922eed7..c8e25bdd197 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2586,18 +2586,17 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, * in increasing order, but it's not clear that there would beenough
* gain to justify the restriction. */ - int start = 0, - end = hscan->rs_ntuples - 1; + uint32 start = 0, end = hscan->rs_ntuples;- while (start <= end) + while (start < end) { - int mid = (start + end) / 2; + int mid = (start + end) / 2; OffsetNumber curoffset = hscan->rs_vistuples[mid];if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}I pushed the straightforward option for now so that it's fixed.
How about:
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index 2da4e4da13..1620f0c3db 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2574,7 +2574,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer, if (scan->rs_flags & SO_ALLOW_PAGEMODE)
{
- uint32 start,
+ int64 start,
end;
if (hscan->rs_ntuples == 0)
@@ -2594,7 +2594,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
while (start <= end)
{
- uint32 mid = (start + end) / 2;
+ int64 mid = (start + end) >> 1;
OffsetNumber curoffset = hscan->rs_vistuples[mid];
if (tupoffset == curoffset)
best regards
Ranier Vilela
Show quoted text
- Melanie
On Wed, Dec 18, 2024 at 9:50 PM Richard Guo <guofenglinux@gmail.com> wrote:
On Thu, Dec 19, 2024 at 8:18 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:I pushed the straightforward option for now so that it's fixed.
I think this binary search code now has a risk of underflow. If 'mid'
is calculated as zero, the second 'if' branch will cause 'end' to
underflow.
Thanks, Richard!
Maybe we need to do something like below.
--- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2600,7 +2600,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, if (tupoffset == curoffset) return true; else if (tupoffset < curoffset) + { + if (mid == 0) + return false; end = mid - 1; + } else start = mid + 1; }Alternatively, we can revert 'start' and 'end' to signed int as they
were before.
What about this instead:
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index 2da4e4da13e..fb90fd869c2 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2574,11 +2574,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer, if (scan->rs_flags & SO_ALLOW_PAGEMODE)
{
- uint32 start,
- end;
-
- if (hscan->rs_ntuples == 0)
- return false;
+ uint32 start = 0,
+ end = hscan->rs_ntuples; /*
* In pageatatime mode, heap_prepare_pagescan() already did visibility
@@ -2589,10 +2586,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- start = 0;
- end = hscan->rs_ntuples - 1;
- while (start <= end)
+ while (start < end)
{
uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];
@@ -2600,7 +2595,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}
Or to make it easier to read, here:
uint32 start = 0,
end = hscan->rs_ntuples;
while (start < end)
{
uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];
if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
end = mid;
else
start = mid + 1;
}
I think this gets rid of any subtraction and is still the same.
- Melanie
On Fri, Dec 20, 2024 at 7:50 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Wed, Dec 18, 2024 at 9:50 PM Richard Guo <guofenglinux@gmail.com> wrote:
I think this binary search code now has a risk of underflow. If 'mid'
is calculated as zero, the second 'if' branch will cause 'end' to
underflow.
What about this instead:
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 2da4e4da13e..fb90fd869c2 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2574,11 +2574,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,if (scan->rs_flags & SO_ALLOW_PAGEMODE) { - uint32 start, - end; - - if (hscan->rs_ntuples == 0) - return false; + uint32 start = 0, + end = hscan->rs_ntuples;/*
* In pageatatime mode, heap_prepare_pagescan() already did visibility
@@ -2589,10 +2586,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- start = 0;
- end = hscan->rs_ntuples - 1;- while (start <= end)
+ while (start < end)
{
uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];
@@ -2600,7 +2595,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}Or to make it easier to read, here:
uint32 start = 0,
end = hscan->rs_ntuples;while (start < end)
{
uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
end = mid;
else
start = mid + 1;
}I think this gets rid of any subtraction and is still the same.
Looks correct to me.
BTW, I kind of doubt that the overflow risk fixed in 28328ec87 is a
real issue in real-world scenarios. Is it realistically possible to
have more than INT_MAX tuples fit on one heap page?
If it is, then the statement below could also be prone to overflow.
uint32 mid = (start + end) / 2;
Maybe you want to change it to:
uint32 mid = start + (end - start) / 2;
Thanks
Richard
Em qui., 19 de dez. de 2024 às 19:50, Melanie Plageman <
melanieplageman@gmail.com> escreveu:
On Wed, Dec 18, 2024 at 9:50 PM Richard Guo <guofenglinux@gmail.com>
wrote:On Thu, Dec 19, 2024 at 8:18 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:I pushed the straightforward option for now so that it's fixed.
I think this binary search code now has a risk of underflow. If 'mid'
is calculated as zero, the second 'if' branch will cause 'end' to
underflow.Thanks, Richard!
Maybe we need to do something like below.
--- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2600,7 +2600,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Bufferbuffer,
if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
+ {
+ if (mid == 0)
+ return false;
end = mid - 1;
+ }
else
start = mid + 1;
}Alternatively, we can revert 'start' and 'end' to signed int as they
were before.What about this instead:
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 2da4e4da13e..fb90fd869c2 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -2574,11 +2574,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,if (scan->rs_flags & SO_ALLOW_PAGEMODE) { - uint32 start, - end; - - if (hscan->rs_ntuples == 0) - return false; + uint32 start = 0, + end = hscan->rs_ntuples;/*
* In pageatatime mode, heap_prepare_pagescan() already did
visibility
@@ -2589,10 +2586,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
* in increasing order, but it's not clear that there would be
enough
* gain to justify the restriction.
*/
- start = 0;
- end = hscan->rs_ntuples - 1;- while (start <= end)
+ while (start < end)
{
uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];
@@ -2600,7 +2595,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}Or to make it easier to read, here:
uint32 start = 0,
end = hscan->rs_ntuples;while (start < end)
{
uint32 mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
end = mid;
else
start = mid + 1;
}I think this gets rid of any subtraction and is still the same.
Look goods to me.
I think that you propose, can get rid of the early test too.
Note the way we can avoid an overflow in the mid calculation.
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index 9f17baea5d..bd1335276a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2577,9 +2577,6 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
uint32 start,
end;- if (hscan->rs_ntuples == 0)
- return false;
-
/*
* In pageatatime mode, heap_prepare_pagescan() already did visibility
* checks, so just look at the info it left in rs_vistuples[].
@@ -2590,17 +2587,17 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
* gain to justify the restriction.
*/
start = 0;
- end = hscan->rs_ntuples - 1;
+ end = hscan->rs_ntuples;
- while (start <= end)
+ while (start < end)
{
- uint32 mid = (start + end) / 2;
+ uint32 mid = (start + end) >> 1;
OffsetNumber curoffset = hscan->rs_vistuples[mid]; if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}
best regards,
Ranier Vilela
On Thu, Dec 19, 2024 at 9:36 PM Richard Guo <guofenglinux@gmail.com> wrote:
On Fri, Dec 20, 2024 at 7:50 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
Looks correct to me.BTW, I kind of doubt that the overflow risk fixed in 28328ec87 is a
real issue in real-world scenarios. Is it realistically possible to
have more than INT_MAX tuples fit on one heap page?If it is, then the statement below could also be prone to overflow.
uint32 mid = (start + end) / 2;
Maybe you want to change it to:
uint32 mid = start + (end - start) / 2;
I've done this and pushed.
Thanks to you and Ranier for your help in identifying and fixing this!
- Melanie