On Fri, May 8, 2015 at 3:55 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 7 May 2015 at 21:40, Michael Paquier <michael.paquier@gmail.com> wrote: >> >> Hi all, >> >> Coverity is complaining about the following assertion introduced in >> commit 924bcf4 (parallel stuff, SerializeSnapshot@snapmgr.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/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services