Обсуждение: pgsql: Avoid -Wconversion warnings when using checksum_impl.h
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. Author: David Steele Discussion: https://postgr.es/m/91d86c8a-11fc-7b88-43eb-5ca3f6fb8bd3@pgmasters.net Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/0065174324a97c0f39ccf0823a81fb674fb49cca Modified Files -------------- src/include/storage/checksum_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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