pgsql: Avoid -Wconversion warnings when using checksum_impl.h

Hi Michael,

On 3/5/20 12:13 AM, Michael Paquier wrote:

Avoid -Wconversion warnings when using checksum_impl.h

This does not matter much when compiling Postgres proper as many
warnings exist when enabling this compilation flag, but it can be
annoying for external modules willing to use both.

Looks like you changed 65535 -> 65536 before commit. I checked the
original patch and it has 65535.

Bit of a bummer that this passed make check-world, but the pgBackRest
tests definitely did not like it.

Thanks,
--
-David
david@pgmasters.net

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.

Bit of a bummer that this passed make check-world, but the pgBackRest tests
definitely did not like it.

Is that because you have a copy of this routine in your code?
--
Michael

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?

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

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

On 3/5/20 6:30 PM, Michael Paquier wrote:

On Thu, Mar 05, 2020 at 12:51:50PM -0500, David Steele wrote:

Bit of a bummer that this passed make check-world, but the pgBackRest tests
definitely did not like it.

Is that because you have a copy of this routine in your code?

Yes, we pull this file into pgBackRest for our checksum validation. We
also do unit and integration tests against it.

Regards,
--
-David
david@pgmasters.net

On Thu, Mar 05, 2020 at 07:32:59PM -0500, David Steele wrote:

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

By doing so, the tests still fail if the page size is something else
than 8k, no?
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:

On Thu, Mar 05, 2020 at 07:32:59PM -0500, David Steele wrote:

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

By doing so, the tests still fail if the page size is something else
than 8k, no?

Of course, but we check that from pg_controldata before we actually
start doing anything with the cluster.

Even a test which at least validated that the checksum code was
returning the right value when page size is the (rather common...) 8K
would be better than a change like this not breaking any tests..

Thanks,

Stephen

On Thu, Mar 05, 2020 at 04:14:20PM -0800, Magnus Hagander wrote:

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?

Yes.. Using pg_checksums --check on an upgraded instance which had
checksums enabled would detect that immediately, but it could be
possible also to have a SQL-callable function which takes in input a
bytea and returns the checksum. In order to make such tests reliable
with any page size, we could pass down the page size to
pg_checksum_page(), and then give the function's caller this
possibility. However I recall that we'd rather keep BLCKSZ hardcoded
in checksum_impl.h on performance grounds.
--
Michael

On 3/5/20 7:48 PM, Michael Paquier wrote:

On Thu, Mar 05, 2020 at 04:14:20PM -0800, Magnus Hagander wrote:

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?

Yes.. Using pg_checksums --check on an upgraded instance which had
checksums enabled would detect that immediately, but it could be
possible also to have a SQL-callable function which takes in input a
bytea and returns the checksum. In order to make such tests reliable
with any page size, we could pass down the page size to
pg_checksum_page(), and then give the function's caller this
possibility. However I recall that we'd rather keep BLCKSZ hardcoded
in checksum_impl.h on performance grounds.

Yeah, keeping BLKSZ a constant is pretty important for performance.
That's one of the main reasons that we only support the default.

Regards,
--
-David
david@pgmasters.net

On Thu, Mar 05, 2020 at 07:58:50PM -0500, David Steele wrote:

Yeah, keeping BLKSZ a constant is pretty important for performance. That's
one of the main reasons that we only support the default.

Doing something is not complicated, now how would people like to do
it? Here are the options I can think of:
1) A TAP test, which bypasses the tests if the page size is not 8k.
2) A test for src/test/regress/, with a method similar to what we do
for collate.sql for the compilations without ICU.

In both cases mentioned here, we would need a SQL function to handle
the work.
--
Michael

On 3/5/20 8:10 PM, Michael Paquier wrote:

On Thu, Mar 05, 2020 at 07:58:50PM -0500, David Steele wrote:

Yeah, keeping BLKSZ a constant is pretty important for performance. That's
one of the main reasons that we only support the default.

Doing something is not complicated, now how would people like to do
it? Here are the options I can think of:
1) A TAP test, which bypasses the tests if the page size is not 8k.
2) A test for src/test/regress/, with a method similar to what we do
for collate.sql for the compilations without ICU.

In both cases mentioned here, we would need a SQL function to handle
the work.

Couldn we use pageinspect?

--
-David
david@pgmasters.net

On Thu, Mar 05, 2020 at 08:20:23PM -0500, David Steele wrote:

Couldn't we use pageinspect?

Oh, indeed. I forgot that we already have this function. The tests
of page.sql don't care about the page size either, so it would be
enough to add a couple of calls with some hardcoded bytea input.
--
Michael

Hi,

On 2020-03-05 12:51:50 -0500, David Steele wrote:

On 3/5/20 12:13 AM, Michael Paquier wrote:

Avoid -Wconversion warnings when using checksum_impl.h

This does not matter much when compiling Postgres proper as many
warnings exist when enabling this compilation flag, but it can be
annoying for external modules willing to use both.

Looks like you changed 65535 -> 65536 before commit. I checked the original
patch and it has 65535.

I gotta say, that's something that just shouldn't happen.

Greetings,

Andres Freund