Thread: Make pg_checksums complain if compiled BLCKSZ and data folder'sblock size differ

Hi all,
(related folks in CC)

Sergei Kornilov has reported here an issue with pg_checksums:
https://www.postgresql.org/message-id/5217311552474471@myt2-66bcb87429e6.qloud-c.yandex.net

If the block size the tool is compiled with does not match the data
folder block size, then users would get incorrect checksums failures,
which is confusing.  As pg_checksum_block() uses directly the block
size, this cannot really be made dynamic yet, so we had better issue
an error on that.  Michael Banck has sent a patch for that:
https://www.postgresql.org/message-id/1552476561.4947.67.camel@credativ.de

The error message proposed is like that:
+   if (ControlFile->blcksz != BLCKSZ)
+   {
+       fprintf(stderr, _("%s: data directory block size %d is different to compiled-in block size %d.\n"),
+               progname, ControlFile->blcksz, BLCKSZ);
+       exit(1);
+   }
Still I think that we could do better.

Here is a proposal of message which looks more natural to me, and more
consistent with what xlog.c complains about:
database files are incompatible with pg_checksums.
The database cluster was initialized with BLCKSZ %d, but pg_checksums
was compiled with BLCKSZ %d.

Has somebody a better wording for that?  Attached is a proposal of
patch.
--
Michael

Bonjour Michaël,

> If the block size the tool is compiled with does not match the data
> folder block size, then users would get incorrect checksums failures,

Or worse, incorrect checksump writing under "enabling"?

Initial proposal:

  "%s: data directory block size %d is different to compiled-in block size %d.\n"

> Has somebody a better wording for that?  Attached is a proposal of
> patch.

  "%s: database files are incompatible with pg_checksums.\n"
  "%s: The database cluster was initialized with BLCKSZ %u, but pg_checksums was compiled with BLCKSZ %u."

Second line is missing a "\n". "pg_checksums" does not need to appear, it 
is already the progname, and if it differs there is no point in giving a 
wrong name. I think it could be shorter. What about:

  "%s: cannot compute checksums, command compiled with BLCKSZ %u but cluster initialized with BLCKSZ %u.\n"

I think it would be better to adapt the checksum computation, but this is 
indeed non trivial because of the way the BLCKSZ constant is hardwired 
into type declarations.

-- 
Fabien.

On Sat, Mar 16, 2019 at 09:18:34AM +0100, Fabien COELHO wrote:
>> If the block size the tool is compiled with does not match the data
>> folder block size, then users would get incorrect checksums failures,
>
> Or worse, incorrect checksump writing under "enabling"?

Let's hope that we make that possible for v12.  We'll see.

> Second line is missing a "\n". "pg_checksums" does not need to appear, it is
> already the progname, and if it differs there is no point in giving a wrong
> name. I think it could be shorter. What about:

Something like "%s: database folder is incompatible" for the first
line sounds kind of better per the feedback gathered.  And then on the
second line:
"The database cluster was initialized with block size %u, but
pg_checksums was compiled with block size %u."

> I think it would be better to adapt the checksum computation, but this is
> indeed non trivial because of the way the BLCKSZ constant is hardwired into
> type declarations.

That's actually the possibility I was pointing out upthread.  I am not
sure that the use cases are worth the effort though.
--
Michael

> Something like "%s: database folder is incompatible" for the first
> line sounds kind of better per the feedback gathered.  And then on the
> second line:
> "The database cluster was initialized with block size %u, but
> pg_checksums was compiled with block size %u."

Ok. "%s" progname instead of "pg_checksums", or just "the command"?

I'm not sure that prefixing the two lines with the comment line is very 
elegant, I'd suggest to put spaces, and would still try to shorten the 
second sentence, maybe:

%s: incompatible database cluster of block size %u, while the command
   is compiled for block size %u.

>> I think it would be better to adapt the checksum computation, but this is
>> indeed non trivial because of the way the BLCKSZ constant is hardwired into
>> type declarations.
>
> That's actually the possibility I was pointing out upthread.

Yes, I was expressing my agreement.

> I am not sure that the use cases are worth the effort though.

Indeed, not for "pg_checksums" only.

A few years I considered to have an dynamic initdb-set block size, but 
BLCKSZ is used a lot as a constant for struct declaration and to compute 
other constants, so that was a lot of changes. I think it would be worth 
the effort because the current page size is suboptimal especially on SSD 
where 4 KiB would provide over 10% better performance for OLTP load. 
However, having to recompile to change it is a pain and not very package 
friendly.

-- 
Fabien.

On Sun, Mar 17, 2019 at 09:17:02AM +0100, Fabien COELHO wrote:
> I'm not sure that prefixing the two lines with the comment line is very
> elegant, I'd suggest to put spaces, and would still try to shorten the
> second sentence, maybe:

I suggest to keep two lines, and only prefix the first one.
--
Michael

On Sat, Mar 16, 2019 at 11:18:17AM +0100, Magnus Hagander wrote:
> BLCKSZ is very much an internal term. The exposed name through pg_settings
> is block_size, so I think the original was better. Combining that one with
> yours into  "initialized with block size %d" etc, makes it a lot nicer.

Yes, what Fabien and you say here makes sense.

> The "incompatible with pg_checksums" part may be a bit redundant with the
> commandname at the start as well, as I now realized Fabien pointed out
> downthread. But I would suggest just cutting it and saying "%s: database
> files are incompatible" or maybe "%s: data directory is incompatible" even?

"Cluster" is more consistent with the surroundings.  So what about the
attached then?
--
Michael

>> I'm not sure that prefixing the two lines with the comment line is very
>> elegant, I'd suggest to put spaces, and would still try to shorten the
>> second sentence, maybe:
>
> I suggest to keep two lines, and only prefix the first one.

As you feel. For me the shorter the better, though, if the information is 
clear and all there.

-- 
Fabien.

On Sun, Mar 17, 2019 at 12:01:31PM +0100, Magnus Hagander wrote:
> LGTM.

Okay, committed.
--
Michael