From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Simon Riggs <simon(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData |
Date: | 2015-05-08 12:02:04 |
Message-ID: | CAB7nPqRZc5uY-=5rJNCAv7qBeTVD=Qyteh9RfLN52aKUXE=8ag@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
Regards,
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Fabien COELHO | 2015-05-08 12:02:23 | Re: PATCH: remove nclients/nthreads constraint from pgbench |
Previous Message | Stephen Frost | 2015-05-08 11:15:58 | Re: Obsolete mention of src/tools/backend |