public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Tom Lane <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: Maxim Orlov <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: wenhui qiu <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: POC: make mxidoff 64 bits
Date: Mon, 15 Dec 2025 11:55:10 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

On 15/12/2025 00:55, Tom Lane wrote:
> Heikki Linnakangas <[email protected]> writes:
>> Ok, I have pushed this. Thanks!
> 
> Coverity is unhappy about this bit:
> 
> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_upgrade/multixact_read_v18.c: 282             in GetOldMultiXactIdSingleMember()
> 276             if (!TransactionIdIsValid(*xactptr))
> 277             {
> 278                 /*
> 279                  * Corner case 2: we are looking at unused slot zero
> 280                  */
> 281                 if (offset == 0)
>>>>      CID 1676077:         Control flow issues  (DEADCODE)
>>>>      Execution cannot reach this statement: "continue;".
> 282                     continue;
> 283
> 284                 /*
> 285                  * Otherwise this is an invalid entry that should not be
> 
> It sees the earlier test for offset == 0, and evidently is assuming
> that the loop's "offset++" will not wrap around.  Now I think that
> the point of this check is exactly that "offset++" could have wrapped
> around, but the commentary is not so clear that I'm certain this is a
> false positive.

Correct.

> If that is the intention, what do you think of rephrasing this
> comment as "we have wrapped around to unused slot zero"?
Ah yes, that's much better. Changed it to "offset must have wrapped 
around to unused slot zero". This code and its comments are copied from 
v18 GetMultiXactIdMembers(), so in order to maintain the rhyme with 
that, I changed the comment in backbranches too. It doesn't exist in the 
'master' version of GetMultiXactIdMembers() anymore.

Coverity is also complaining about the 'length' variable being tainted, 
because it's calculated from the values read from disk. That's bogus 
because we trust and make assumptions of the values on disk. That said, 
I think it would make sense to do some more sanity checking here. In 
particular, length should never be negative. I added such sanity checks 
to the 'master' version of the GetMultiXactIdMembers() server function 
in commit d4b7bde418, but it would make sense to add them to the upgrade 
code too. I'll look into that.

- Heikki

view thread (79+ messages)  latest in thread

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], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: POC: make mxidoff 64 bits
  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