Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

On 8 May 2015 at 13:02, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> On Fri, May 8, 2015 at 3:55 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On 7 May 2015 at 21:40, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
> >>
> >> Hi all,
> >>
> >> Coverity is complaining about the following assertion introduced in
> >> commit 924bcf4 (parallel stuff, SerializeSnapshot(at)snapmgr(dot)c):
> >> + Assert(snapshot->xcnt >= 0);
> >>
> >> Now the thing is that this assertion does not make much sense, because
> >> SnapshotData defines subxcnt as uint32 in snapshot.h. While we could
> >> simply remove this assertion, I am wondering if we could not change
> >> subxcnt to uint32 instead.
> >>
> >> SnapshotData has been introduced in 2008 by d43b085, with this comment:
> >> + int32 subxcnt; /* # of xact ids in
> >> subxip[], -1 if overflow */
> >> Comment regarding negative values removed in efc16ea5.
> >>
> >> Now, by looking at the code on HEAD, I am seeing no code paths that
> >> make use of negative values of subxcnt. Perhaps I am missing
> >> something?
> >
> >
> > So the comment is wrong? It does not set to -1 at overflow anymore?
>
> SnapshotData.suboverflowed is used instead. Have a look at efc16ea5 in
> procarray.c to convince yourself:
>
> @@ -785,16 +1121,17 @@ GetSnapshotData(Snapshot snapshot)
> *
> * Again, our own XIDs are not included in the snapshot.
> */
> - if (subcount >= 0 && proc != MyProc)
> + if (!suboverflowed && proc != MyProc)
> {
> if (proc->subxids.overflowed)
> - subcount = -1; /* overflowed */
> + suboverflowed = true;
> else
>
> I think that we should redefine subxcnt as uint32 for consistency with
> xcnt, and remove the two assertions that 924bcf4 has introduced. I
> could get a patch quickly done FWIW.
>

(uint32) +1

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services