Re: [PATCH v1] remove redundant check of item pointer

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(at)gmail(dot)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(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

Indecision is a decision. Inaction is an action. Mark Batterson