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@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