subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

Started by Michael Paquierover 10 years ago9 messages

michael.paquier@gmail.com

over 10 years ago

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs

simon@2ndQuadrant.com

over 10 years ago

In reply to: Michael Paquier (#1)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Simon Riggs (#2)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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.
Regards,
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs

simon@2ndQuadrant.com

over 10 years ago

In reply to: Michael Paquier (#3)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Simon Riggs (#4)

1 attachment(s)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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

Attachments:

20150509_subxcnt_uint.patchapplication/x-patch; name=20150509_subxcnt_uint.patchDownload

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index a2cb4a0..e1ca52c 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -168,7 +168,7 @@ typedef struct SerializedSnapshotData
 	TransactionId xmin;
 	TransactionId xmax;
 	uint32		xcnt;
-	int32		subxcnt;
+	uint32		subxcnt;
 	bool		suboverflowed;
 	bool		takenDuringRecovery;
 	CommandId	curcid;
@@ -1480,9 +1480,6 @@ SerializeSnapshot(Snapshot snapshot, char *start_address)
 {
 	SerializedSnapshotData *serialized_snapshot;
 
-	Assert(snapshot->xcnt >= 0);
-	Assert(snapshot->subxcnt >= 0);
-
 	serialized_snapshot = (SerializedSnapshotData *) start_address;
 
 	/* Copy all required fields */
diff --git a/src/include/utils/snapshot.h b/src/include/utils/snapshot.h
index a734bf0..d37ff0b 100644
--- a/src/include/utils/snapshot.h
+++ b/src/include/utils/snapshot.h
@@ -85,7 +85,7 @@ typedef struct SnapshotData
 	 * out any that are >= xmax
 	 */
 	TransactionId *subxip;
-	int32		subxcnt;		/* # of xact ids in subxip[] */
+	uint32		subxcnt;		/* # of xact ids in subxip[] */
 	bool		suboverflowed;	/* has the subxip array overflowed? */
 
 	bool		takenDuringRecovery;	/* recovery-shaped snapshot? */

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Michael Paquier (#5)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Robert Haas (#6)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Michael Paquier (#7)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Michael Paquier (#8)

Re: subxcnt defined as signed integer in SnapshotData and SerializeSnapshotData

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers