Re: [HACKERS] 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