Re: [PATCH] Missing Assert in the code

Hello

On 2024-Nov-26, Dmitry Nikitin wrote:

> Monday, November 25, 2024, 10:51:31 PM, you wrote:
>
> AH> Hmm, I think if we believe this to be really possible, we should have an
> AH> 'if/elog' test (or maybe a full ereport with ERRCODE_DATA_CORRUPTED
> AH> errcode) rather than an assertion. I think the assertion adds nothing
> AH> of value here, but maybe an 'if' test would.

> I don't mind of 'if' however it's not clear for me what to follow on
> other (error) branch of that 'if'.

An elog(ERROR) doesn't continue execution of the function (it's a
longjmp to transaction abort and error recovery), so you don't have to
worry about that.

My point is that an Assert() is only executed in a debug build;
production builds don't have assertions.

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

> Mostly it used in loops. So some code will be gracefully skipped and
> that's it.

Yeah, I noticed that in a few that I looked at, but it's over a hundred
of them.

> However the case discussed is different because -1 index of an array
> will be accessed. Which is much worse than a bare assertion at least.

Sure.

By the way, how did you find out about this problem? Is this
hypothetical or was there an actual crash? AFAICS (some of?) the
callers get to this code immediately after executing GinInitPage, so
it's not clear to me that there's an actual bug here.

... Ah, maybe ginFindLeafPage could be at risk, through ->isMoveRight.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)