Re: [HACKERS] Fix a typo in snapmgr.c

[Simon Riggs]

On 23 June 2017 at 19:25, Andres Freund  wrote:
> On 2017-06-23 19:21:57 +0100, Simon Riggs wrote:
>> On 23 June 2017 at 08:23, Simon Riggs  wrote:
>> > On 23 June 2017 at 08:21, Andres Freund  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  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,
>_snapshot, sizeof(SerializedSnapshotData));
>
> i.e. if suboverflowed, start_address would contain subxcnt = 0.
>
> New code:
>
>
> /* Copy struct to possibly-unaligned buffer */
> memcpy(start_address,
>_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 Riggshttp://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

Re: [HACKERS] Fix a typo in snapmgr.c

[Andres Freund]

On 2017-06-23 19:21:57 +0100, Simon Riggs wrote:
> On 23 June 2017 at 08:23, Simon Riggs  wrote:
> > On 23 June 2017 at 08:21, Andres Freund  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  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,
   _snapshot, sizeof(SerializedSnapshotData));

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

New code:


/* Copy struct to possibly-unaligned buffer */
memcpy(start_address,
   _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

Re: [HACKERS] Fix a typo in snapmgr.c

[Simon Riggs]

On 23 June 2017 at 08:23, Simon Riggs  wrote:
> On 23 June 2017 at 08:21, Andres Freund  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  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 Riggshttp://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

Re: [HACKERS] Fix a typo in snapmgr.c

[Simon Riggs]

On 23 June 2017 at 08:21, Andres Freund  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  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 Riggshttp://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

Re: [HACKERS] Fix a typo in snapmgr.c

[Andres Freund]

On 2017-06-07 10:17:31 -0700, Andres Freund wrote:
> On 2017-05-08 09:12:13 -0400, Tom Lane wrote:
> > Simon Riggs  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

Re: [HACKERS] Fix a typo in snapmgr.c

[Masahiko Sawada]

On Thu, Jun 8, 2017 at 2:17 AM, Andres Freund  wrote:
> On 2017-05-08 09:12:13 -0400, Tom Lane wrote:
>> Simon Riggs  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

Re: [HACKERS] Fix a typo in snapmgr.c

[Andres Freund]

On 2017-05-08 09:12:13 -0400, Tom Lane wrote:
> Simon Riggs  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

Re: [HACKERS] Fix a typo in snapmgr.c

[Masahiko Sawada]

On Mon, May 8, 2017 at 10:12 PM, Tom Lane  wrote:
> Simon Riggs  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

Re: [HACKERS] Fix a typo in snapmgr.c

[Tom Lane]

Simon Riggs  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

Re: [HACKERS] Fix a typo in snapmgr.c

[Masahiko Sawada]

On Mon, May 8, 2017 at 4:57 PM, Simon Riggs  wrote:
> On 8 May 2017 at 06:28, Masahiko Sawada  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

Re: [HACKERS] Fix a typo in snapmgr.c

[Simon Riggs]

On 8 May 2017 at 06:28, Masahiko Sawada  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 Riggshttp://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

[HACKERS] Fix a typo in snapmgr.c

[Masahiko Sawada]

Hi,

Attached patch for $subject.

s/recovey/recovery/

Regards,

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


fix_typo_in_snapmgr_c.patch
Description: Binary data

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