Обсуждение: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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

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

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

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

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

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

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