Fix a typo in snapmgr.c

Started by Masahiko Sawadaover 8 years ago12 messages

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

1 attachment(s)

Hi,

Attached patch for $subject.

s/recovey/recovery/

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachments:

fix_typo_in_snapmgr_c.patchapplication/octet-stream; name=fix_typo_in_snapmgr_c.patchDownload

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 3a3a25a..1363f33 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -2041,7 +2041,7 @@ SerializeSnapshot(Snapshot snapshot, char *start_address)
 
 	/*
 	 * Ignore the SubXID array if it has overflowed, unless the snapshot was
-	 * taken during recovey - in that case, top-level XIDs are in subxip as
+	 * taken during recovery - in that case, top-level XIDs are in subxip as
 	 * well, and we mustn't lose them.
 	 */
 	if (serialized_snapshot.suboverflowed && !snapshot->takenDuringRecovery)

Simon Riggs

simon@2ndquadrant.com

over 8 years ago

In reply to: Masahiko Sawada (#1)

Re: Fix a typo in snapmgr.c

On 8 May 2017 at 06:28, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

Attached patch for $subject.

s/recovey/recovery/

There was a typo, but the comment itself was slightly wrong and also
duplicated with another comment further down.

So rearranged code a little to keep it lean.

Thanks

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Simon Riggs (#2)

Re: Fix a typo in snapmgr.c

On Mon, May 8, 2017 at 4:57 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 8 May 2017 at 06:28, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

Attached patch for $subject.

s/recovey/recovery/

There was a typo, but the comment itself was slightly wrong and also
duplicated with another comment further down.

So rearranged code a little to keep it lean.

Okay, thank you!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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

Tom Lane

tgl@sss.pgh.pa.us

over 8 years ago

In reply to: Simon Riggs (#2)

Re: Fix a typo in snapmgr.c

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

regards, tom lane

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Tom Lane (#4)

Re: Fix a typo in snapmgr.c

On Mon, May 8, 2017 at 10:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

Right, I'd missed it. That code should be placed before first memcpy.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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

Andres Freund

andres@anarazel.de

over 8 years ago

In reply to: Tom Lane (#4)

Re: Fix a typo in snapmgr.c

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

- Andres

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Andres Freund (#6)

Re: Fix a typo in snapmgr.c

On Thu, Jun 8, 2017 at 2:17 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

I think we don't have the fix for the comment from Tom so far, too.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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

Andres Freund

andres@anarazel.de

over 8 years ago

In reply to: Andres Freund (#6)

Re: Fix a typo in snapmgr.c

On 2017-06-07 10:17:31 -0700, Andres Freund wrote:

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

Simon, FWIW, I plan to either revert or fix this up soon-ish. Unless
you're going to actually respond on this thread?

- Andres

--
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 8 years ago

In reply to: Andres Freund (#8)

Re: Fix a typo in snapmgr.c

On 23 June 2017 at 08:21, Andres Freund <andres@anarazel.de> wrote:

On 2017-06-07 10:17:31 -0700, Andres Freund wrote:

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

Simon, FWIW, I plan to either revert or fix this up soon-ish. Unless
you're going to actually respond on this thread?

Sorry, I've only just seen Tom's reply. Will fix.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

#10

Simon Riggs

simon@2ndquadrant.com

over 8 years ago

In reply to: Simon Riggs (#9)

Re: Fix a typo in snapmgr.c

On 23 June 2017 at 08:23, Simon Riggs <simon@2ndquadrant.com> wrote:

On 23 June 2017 at 08:21, Andres Freund <andres@anarazel.de> wrote:

On 2017-06-07 10:17:31 -0700, Andres Freund wrote:

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

Simon, FWIW, I plan to either revert or fix this up soon-ish. Unless
you're going to actually respond on this thread?

Sorry, I've only just seen Tom's reply. Will fix.

I don't see a bug. Perhaps I'm tired and can't see it yet.

Will fix if you thwack me with the explanation.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

#11

Andres Freund

andres@anarazel.de

over 8 years ago

In reply to: Simon Riggs (#10)

Re: Fix a typo in snapmgr.c

On 2017-06-23 19:21:57 +0100, Simon Riggs wrote:

On 23 June 2017 at 08:23, Simon Riggs <simon@2ndquadrant.com> wrote:

On 23 June 2017 at 08:21, Andres Freund <andres@anarazel.de> wrote:

On 2017-06-07 10:17:31 -0700, Andres Freund wrote:

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

Simon, FWIW, I plan to either revert or fix this up soon-ish. Unless
you're going to actually respond on this thread?

Sorry, I've only just seen Tom's reply. Will fix.

I don't see a bug. Perhaps I'm tired and can't see it yet.

Will fix if you thwack me with the explanation.

Wasn't my complaint, but here we go:

Previous code:

/*
* Ignore the SubXID array if it has overflowed, unless the snapshot was
* taken during recovey - in that case, top-level XIDs are in subxip as
* well, and we mustn't lose them.
*/
if (serialized_snapshot.suboverflowed && !snapshot->takenDuringRecovery)
serialized_snapshot.subxcnt = 0;

/* Copy struct to possibly-unaligned buffer */
memcpy(start_address,
&serialized_snapshot, sizeof(SerializedSnapshotData));

i.e. if suboverflowed, start_address would contain subxcnt = 0.

New code:

/* Copy struct to possibly-unaligned buffer */
memcpy(start_address,
&serialized_snapshot, sizeof(SerializedSnapshotData));

/* Copy XID array */
if (snapshot->xcnt > 0)
memcpy((TransactionId *) (start_address +
sizeof(SerializedSnapshotData)),
snapshot->xip, snapshot->xcnt * sizeof(TransactionId));

/*
* Copy SubXID array. Don't bother to copy it if it had overflowed,
* though, because it's not used anywhere in that case. Except if it's a
* snapshot taken during recovery; all the top-level XIDs are in subxip as
* well in that case, so we mustn't lose them.
*/
if (serialized_snapshot.suboverflowed && !snapshot->takenDuringRecovery)
serialized_snapshot.subxcnt = 0;

Here the copy is done before subxcnt = 0.

- Andres

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

#12

Simon Riggs

simon@2ndquadrant.com

over 8 years ago

In reply to: Andres Freund (#11)

Re: Fix a typo in snapmgr.c

On 23 June 2017 at 19:25, Andres Freund <andres@anarazel.de> wrote:

On 2017-06-23 19:21:57 +0100, Simon Riggs wrote:

On 23 June 2017 at 08:23, Simon Riggs <simon@2ndquadrant.com> wrote:

On 23 June 2017 at 08:21, Andres Freund <andres@anarazel.de> wrote:

On 2017-06-07 10:17:31 -0700, Andres Freund wrote:

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

So rearranged code a little to keep it lean.

Didn't you break it with that? As it now stands, the memcpy will
copy the nonzero value.

I've not seen a fix and/or alleviating comment about this so far. Did I
miss something?

Simon, FWIW, I plan to either revert or fix this up soon-ish. Unless
you're going to actually respond on this thread?

Sorry, I've only just seen Tom's reply. Will fix.

I don't see a bug. Perhaps I'm tired and can't see it yet.

Will fix if you thwack me with the explanation.

Wasn't my complaint, but here we go:

Previous code:

/*
* Ignore the SubXID array if it has overflowed, unless the snapshot was
* taken during recovey - in that case, top-level XIDs are in subxip as
* well, and we mustn't lose them.
*/
if (serialized_snapshot.suboverflowed && !snapshot->takenDuringRecovery)
serialized_snapshot.subxcnt = 0;

/* Copy struct to possibly-unaligned buffer */
memcpy(start_address,
&serialized_snapshot, sizeof(SerializedSnapshotData));

i.e. if suboverflowed, start_address would contain subxcnt = 0.

New code:

/* Copy struct to possibly-unaligned buffer */
memcpy(start_address,
&serialized_snapshot, sizeof(SerializedSnapshotData));

/* Copy XID array */
if (snapshot->xcnt > 0)
memcpy((TransactionId *) (start_address +
sizeof(SerializedSnapshotData)),
snapshot->xip, snapshot->xcnt * sizeof(TransactionId));

/*
* Copy SubXID array. Don't bother to copy it if it had overflowed,
* though, because it's not used anywhere in that case. Except if it's a
* snapshot taken during recovery; all the top-level XIDs are in subxip as
* well in that case, so we mustn't lose them.
*/
if (serialized_snapshot.suboverflowed && !snapshot->takenDuringRecovery)
serialized_snapshot.subxcnt = 0;

Here the copy is done before subxcnt = 0.

OK, me looking at the wrong memcpy, my bad. Thanks for the thwack.

Fixed.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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