Message-ID: <7b505284-c6e9-4c80-a7ee-816493170abc@iki.fi>
Date: Mon, 15 Dec 2025 11:55:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: POC: make mxidoff 64 bits
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Maxim Orlov <orlovmg@gmail.com>, Alexander Korotkov <aekorotkov@gmail.com>,
 wenhui qiu <qiuwenhuifx@gmail.com>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <202512110804.hmukvd7jna34@alvherre.pgsql>
 <2d9282d3-7f24-4e1c-af5f-cd33316dead7@iki.fi>
 <1827755.1765752936@sss.pgh.pa.us>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <1827755.1765752936@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/7b505284-c6e9-4c80-a7ee-816493170abc%40iki.fi>
Precedence: bulk

On 15/12/2025 00:55, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> 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