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
