public inbox for [email protected]  

help / color / mirror / Atom feed

From: Alvaro Herrera <[email protected]>
To: Dmitry Nikitin <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Missing Assert in the code
Date: Tue, 26 Nov 2024 12:40:39 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

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)

view thread (5+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: [PATCH] Missing Assert in the code
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox