Thread: [PATCH v1] remove redundant check of item pointer
In function ItemPointerEquals, the ItemPointerGetBlockNumber
already checked the ItemPointer if valid, there is no need
to check it again in ItemPointerGetOffset, so use
ItemPointerGetOffsetNumberNoCheck instead.
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
---
src/backend/storage/page/itemptr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/page/itemptr.c b/src/backend/storage/page/itemptr.c
index 9011337aa8..61ad727b1d 100644
--- a/src/backend/storage/page/itemptr.c
+++ b/src/backend/storage/page/itemptr.c
@@ -37,8 +37,8 @@ ItemPointerEquals(ItemPointer pointer1, ItemPointer pointer2)
if (ItemPointerGetBlockNumber(pointer1) ==
ItemPointerGetBlockNumber(pointer2) &&
- ItemPointerGetOffsetNumber(pointer1) ==
- ItemPointerGetOffsetNumber(pointer2))
+ ItemPointerGetOffsetNumberNoCheck(pointer1) ==
+ ItemPointerGetOffsetNumberNoCheck(pointer2))
return true;
else
return false;
--
2.33.0
On Wed, Apr 27, 2022 at 08:04:00PM +0800, Junwang Zhao wrote:
> In function ItemPointerEquals, the ItemPointerGetBlockNumber
> already checked the ItemPointer if valid, there is no need
> to check it again in ItemPointerGetOffset, so use
> ItemPointerGetOffsetNumberNoCheck instead.
>
> Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
> ---
> src/backend/storage/page/itemptr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/storage/page/itemptr.c b/src/backend/storage/page/itemptr.c
> index 9011337aa8..61ad727b1d 100644
> --- a/src/backend/storage/page/itemptr.c
> +++ b/src/backend/storage/page/itemptr.c
> @@ -37,8 +37,8 @@ ItemPointerEquals(ItemPointer pointer1, ItemPointer pointer2)
>
> if (ItemPointerGetBlockNumber(pointer1) ==
> ItemPointerGetBlockNumber(pointer2) &&
> - ItemPointerGetOffsetNumber(pointer1) ==
> - ItemPointerGetOffsetNumber(pointer2))
> + ItemPointerGetOffsetNumberNoCheck(pointer1) ==
> + ItemPointerGetOffsetNumberNoCheck(pointer2))
> return true;
> else
> return false;
Looking at the code:
/*
* ItemPointerGetOffsetNumberNoCheck
* Returns the offset number of a disk item pointer.
*/
static inline OffsetNumber
ItemPointerGetOffsetNumberNoCheck(const ItemPointerData *pointer)
{
return pointer->ip_posid;
}
/*
* ItemPointerGetOffsetNumber
* As above, but verifies that the item pointer looks valid.
*/
static inline OffsetNumber
ItemPointerGetOffsetNumber(const ItemPointerData *pointer)
{
Assert(ItemPointerIsValid(pointer));
return ItemPointerGetOffsetNumberNoCheck(pointer);
}
for non-Assert builds, ItemPointerGetOffsetNumberNoCheck() and
ItemPointerGetOffsetNumber() are the same, so I don't see the point to
making this change. Frankly, I don't know why we even have two
functions for this. I am guessing ItemPointerGetOffsetNumberNoCheck is
for cases where you have an Assert build and do not want the check.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
On Thu, Jul 14, 2022 at 3:31 PM Bruce Momjian <bruce@momjian.us> wrote: > On Wed, Apr 27, 2022 at 08:04:00PM +0800, Junwang Zhao wrote: > for non-Assert builds, ItemPointerGetOffsetNumberNoCheck() and > ItemPointerGetOffsetNumber() are the same, so I don't see the point to > making this change. Frankly, I don't know why we even have two > functions for this. I am guessing ItemPointerGetOffsetNumberNoCheck is > for cases where you have an Assert build and do not want the check. Sometimes we use ItemPointerData for things that aren't actually TIDs. For example, both GIN and B-Tree type-pun the ItemPointerData field from the Indextuple struct. Plus we do something like that with UPDATEs that affect a partitioning key in a partitioned table. The proposal doesn't seem like an improvement. Technically the assertion cannot possibly fail here because the earlier assertion would always fail instead, so strictly speaking it is redundant -- at least right now. That is true. But it seems much more important to be consistent about which variant to use. Especially because there is obviously no overhead in builds without assertions enabled. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> The proposal doesn't seem like an improvement. Technically the
> assertion cannot possibly fail here because the earlier assertion
> would always fail instead, so strictly speaking it is redundant -- at
> least right now. That is true. But it seems much more important to be
> consistent about which variant to use. Especially because there is
> obviously no overhead in builds without assertions enabled.
Even in an assert-enabled build, wouldn't you expect the compiler to
optimize away the second assertion as unreachable code?
regards, tom lane
On Thu, Jul 14, 2022 at 3:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Even in an assert-enabled build, wouldn't you expect the compiler to > optimize away the second assertion as unreachable code? I think that it probably would, even at -O0 (GCC doesn't really allow you to opt out of all optimizations). I did think of that myself, but it seemed rather beside the point. There have been individual cases where individual assertions were deemed a bit too heavyweight. But those have been few and far between. I myself tend to use *lots* of technically-redundant assertions like this for preconditions and postconditions. At worst they're code comments that are all but guaranteed to stay current. -- Peter Geoghegan
On Fri, 15 Jul 2022 at 10:31, Bruce Momjian <bruce@momjian.us> wrote: > for non-Assert builds, ItemPointerGetOffsetNumberNoCheck() and > ItemPointerGetOffsetNumber() are the same, so I don't see the point to > making this change. Frankly, I don't know why we even have two > functions for this. I am guessing ItemPointerGetOffsetNumberNoCheck is > for cases where you have an Assert build and do not want the check. We'll want to use ItemPointerGetOffsetNumberNoCheck() where the TID comes from sources we can't verify. e.g user input... '(2,0)'::tid. We want to use ItemPointerGetOffsetNumber() for item pointers that come from locations that we want to ensure are correct. e.g TIDs we're storing in an index. David