Thread: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Michael Paquier
Date:
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? Regards, -- Michael
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Simon Riggs
Date:
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?
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Michael Paquier
Date:
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 includedin 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
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Simon Riggs
Date:
On 8 May 2015 at 13:02, Michael Paquier <michael.paquier@gmail.com> wrote:
--
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
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Michael Paquier
Date:
On Fri, May 8, 2015 at 10:27 PM, Simon Riggs wrote: > On 8 May 2015 at 13:02, Michael Paquier wrote: >> 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 Attached is the patch. This has finished by being far simpler than what I thought first. Regards, -- Michael
Attachment
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Robert Haas
Date:
On Sat, May 9, 2015 at 8:48 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, May 8, 2015 at 10:27 PM, Simon Riggs wrote: >> On 8 May 2015 at 13:02, Michael Paquier wrote: >>> 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 > > Attached is the patch. This has finished by being far simpler than > what I thought first. I'm just going to remove the useless assertion for now. What you're proposing here may (or may not) be worth doing, but it carries a non-zero risk of breaking something somewhere, if anyone is relying on the signed-ness of that type. Removing the assertion is definitely safe. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Michael Paquier
Date:
On Thu, May 14, 2015 at 12:02 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, May 9, 2015 at 8:48 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Fri, May 8, 2015 at 10:27 PM, Simon Riggs wrote: >>> On 8 May 2015 at 13:02, Michael Paquier wrote: >>>> 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 >> >> Attached is the patch. This has finished by being far simpler than >> what I thought first. > > I'm just going to remove the useless assertion for now. What you're > proposing here may (or may not) be worth doing, but it carries a > non-zero risk of breaking something somewhere, if anyone is relying on > the signed-ness of that type. Removing the assertion is definitely > safe. Fine for me. That's indeed possible for an extension. -- Michael
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Michael Paquier
Date:
On Thu, May 14, 2015 at 1:03 AM, Michael Paquier wrote: > On Thu, May 14, 2015 at 12:02 AM, Robert Haas wrote: >> I'm just going to remove the useless assertion for now. What you're >> proposing here may (or may not) be worth doing, but it carries a >> non-zero risk of breaking something somewhere, if anyone is relying on >> the signed-ness of that type. Removing the assertion is definitely >> safe. > > Fine for me. That's indeed possible for an extension. Btw, I think that your commit message should have given some credit to Coverity for finding the problem. Not a big deal though. -- Michael
Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData
From
Robert Haas
Date:
On Wed, May 13, 2015 at 12:17 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, May 14, 2015 at 1:03 AM, Michael Paquier wrote: >> On Thu, May 14, 2015 at 12:02 AM, Robert Haas wrote: >>> I'm just going to remove the useless assertion for now. What you're >>> proposing here may (or may not) be worth doing, but it carries a >>> non-zero risk of breaking something somewhere, if anyone is relying on >>> the signed-ness of that type. Removing the assertion is definitely >>> safe. >> >> Fine for me. That's indeed possible for an extension. > > Btw, I think that your commit message should have given some credit to > Coverity for finding the problem. Not a big deal though. The first report I received was from Andres via IM, actually: it showed up as a compiler warning for him. I just didn't get around to doing anything about it before this one showed up. I could have explained all that in the commit message, but for a one-line change, it didn't quite seem worth having 3 or 4 lines to attribute credit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company