Re: [PATCH] Missing Assert in the code

On 2024-Nov-25, Dmitry Nikitin wrote:

> Subject: [PATCH 2/2] Add Assert to stop invalid values to pass on
> 
> PageGetMaxOffsetNumber() can legitimately return zero
> (InvalidOffsetNumber) as an indication of error. However there are no
> any checks against that. As a result, for exampe, subsequent
> PageGetItemId() will be accessing an array at -1.

> @@ -236,6 +236,8 @@ getRightMostTuple(Page page)
>  {
>      OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
>  
> +    Assert(OffsetNumberIsValid(maxoff));
> +
>      return (IndexTuple) PageGetItem(page, PageGetItemId(page, maxoff));
>  }

Hmm, I think if we believe this to be really possible, we should have an
'if/elog' test (or maybe a full ereport with ERRCODE_DATA_CORRUPTED
errcode) rather than an assertion.  I think the assertion adds nothing
of value here, but maybe an 'if' test would.

Did you examine the other callers of PageGetMaxOffsetNumber()?  It's a
large bunch.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
[…] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es!
A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues!
        Heiligenstädter Testament, L. v. Beethoven, 1802
        https://de.wikisource.org/wiki/Heiligenstädter_Testament