Horton

Thread

Commits

  1. Error out in pg_verify_checksums on incompatible block size

  2. Error out in pg_checksums on incompatible block size

  1. Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Michael Paquier <michael@paquier.xyz> — 2019-03-16T01:21:47Z

    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

  2. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-03-16T08:18:34Z

    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.

  3. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Magnus Hagander <magnus@hagander.net> — 2019-03-16T10:18:17Z

    On Sat, Mar 16, 2019 at 2:22 AM Michael Paquier <michael@paquier.xyz> wrote:

> 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.
>

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.

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?

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

  4. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Michael Paquier <michael@paquier.xyz> — 2019-03-17T05:46:57Z

    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

  5. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-03-17T08:17:02Z

    > 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.

  6. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Michael Paquier <michael@paquier.xyz> — 2019-03-17T08:25:10Z

    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

  7. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Michael Paquier <michael@paquier.xyz> — 2019-03-17T09:10:45Z

    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

  8. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-03-17T09:11:32Z

    >> 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.

  9. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Magnus Hagander <magnus@hagander.net> — 2019-03-17T11:01:31Z

    On Sun, Mar 17, 2019 at 10:10 AM Michael Paquier <michael@paquier.xyz>
wrote:

> 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?
>

LGTM.


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

  10. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Magnus Hagander <magnus@hagander.net> — 2019-03-17T11:03:04Z

    On Sun, Mar 17, 2019 at 6:47 AM Michael Paquier <michael@paquier.xyz> wrote:

> On Sat, Mar 16, 2019 at 09:18:34AM +0100, Fabien COELHO wrote:
> > 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.
>

It may be worthwhile, but I think we shouldn't target that for v12 --
consider it a potential improvement for upcoming version. Let's focus on
the things we have now to make sure we get those polished and applied first.

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

  11. Re: Make pg_checksums complain if compiled BLCKSZ and data folder's block size differ

    Michael Paquier <michael@paquier.xyz> — 2019-03-18T00:14:56Z

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

Okay, committed.
--
Michael