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

[Bruce Momjian]

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 <zhjw...@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  <br...@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

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