Subject: Re: pgsql: Avoid -Wconversion warnings when using checksum_impl.h
To: Magnus Hagander <magnus@hagander.net>,
 Michael Paquier <michael@paquier.xyz>
Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
References: <E1j9ioh-0005Kn-4O@gemulon.postgresql.org>
 <1cf30561-7dad-dc6e-9fc3-5c456948cfeb@pgmasters.net>
 <20200305233014.GA2501@paquier.xyz>
 <CABUevEzoWeR_q-JztqacmATBvVMUL9rtOcpM5RACubKcmhmk3Q@mail.gmail.com>
From: David Steele <david@pgmasters.net>
Message-ID: <7916219a-96c6-4298-cb36-c97c6bd3930c@pgmasters.net>
Date: Thu, 5 Mar 2020 19:32:59 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0)
 Gecko/20100101 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: 
 <CABUevEzoWeR_q-JztqacmATBvVMUL9rtOcpM5RACubKcmhmk3Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/5/20 7:14 PM, Magnus Hagander wrote:
> On Thu, Mar 5, 2020 at 3:30 PM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Thu, Mar 05, 2020 at 12:51:50PM -0500, David Steele wrote:
>>> Looks like you changed 65535 -> 65536 before commit.  I checked the original
>>> patch and it has 65535.
>>
>> That's my fault, thanks.  I have been playing with the surroundings
>> while looking at your patch.
> 
> :/
> 
> That's a pretty dangerous mistake, and moreso because we don't have a
> test around it. We should really find a way to add such a test -- just
> a hardcoded page calculation that should always return the same value
> perhaps?

FWIW, we use static values in our unit tests (which are written in C), 
then test against packaged versions of Postgres for integration tests.

When I saw the commit I pulled it in so I could remove instructions for 
the manual step to add the cast.  So in this case the issue was apparent 
really quickly.  Normally we only pull in new code from PostgreSQL once 
a year.

We think our unit tests against static values may have endianess issues 
but we have not verified that one way or the other.  Here's what they 
look like:

https://github.com/pgbackrest/pgbackrest/blob/e55443c890181ea63a350275447885331c8254e4/test/src/module/postgres/interfaceTest.c#L182

Regards,
-- 
-David
david@pgmasters.net