Thread: [HACKERS] Fix a typo in snapmgr.c
Hi, Attached patch for $subject. s/recovey/recovery/ 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
Attachment
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
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
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
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
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
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
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
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
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
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-levelXIDs 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 wemustn't lose them. */if (serialized_snapshot.suboverflowed && !snapshot->takenDuringRecovery) serialized_snapshot.subxcnt= 0; Here the copy is done before subxcnt = 0. - Andres
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