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

Started by Michael Paquieralmost 7 years ago11 messages

michael@paquier.xyz

almost 7 years ago

1 attachment(s)

Hi all,
(related folks in CC)

Sergei Kornilov has reported here an issue with pg_checksums:
/messages/by-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:
/messages/by-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

Attachments:

checksums-blcksz-error.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5d4083fa9f..423c036c0f 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -327,6 +327,15 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	if (ControlFile->blcksz != BLCKSZ)
+	{
+		fprintf(stderr, _("%s: database files are incompatible with pg_checksums.\n"),
+				progname);
+		fprintf(stderr, _("%s: The database cluster was initialized with BLCKSZ %u, but pg_checksums was compiled with BLCKSZ %u."),
+				progname, ControlFile->blcksz, BLCKSZ);
+		exit(1);
+	}
+
 	if (ControlFile->state != DB_SHUTDOWNED &&
 		ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
 	{

Fabien COELHO

coelho@cri.ensmp.fr

almost 7 years ago

In reply to: Michael Paquier (#1)

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

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.

Magnus Hagander

magnus@hagander.net

almost 7 years ago

In reply to: Michael Paquier (#1)

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

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:

/messages/by-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:
/messages/by-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/>

Michael Paquier

michael@paquier.xyz

almost 7 years ago

In reply to: Fabien COELHO (#2)

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

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

Fabien COELHO

coelho@cri.ensmp.fr

almost 7 years ago

In reply to: Michael Paquier (#4)

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

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.

Michael Paquier

michael@paquier.xyz

almost 7 years ago

In reply to: Fabien COELHO (#5)

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

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

Michael Paquier

michael@paquier.xyz

almost 7 years ago

In reply to: Magnus Hagander (#3)

1 attachment(s)

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

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

Attachments:

checksums-blcksz-error-v2.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5d4083fa9f..b7ebc11017 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -327,6 +327,15 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
+	if (ControlFile->blcksz != BLCKSZ)
+	{
+		fprintf(stderr, _("%s: database cluster is not compatible.\n"),
+				progname);
+		fprintf(stderr, _("The database cluster was initialized with block size %u, but pg_checksums was compiled with block size %u.\n"),
+				ControlFile->blcksz, BLCKSZ);
+		exit(1);
+	}
+
 	if (ControlFile->state != DB_SHUTDOWNED &&
 		ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
 	{

Fabien COELHO

coelho@cri.ensmp.fr

almost 7 years ago

In reply to: Michael Paquier (#6)

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

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.

Magnus Hagander

magnus@hagander.net

almost 7 years ago

In reply to: Michael Paquier (#7)

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

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

Magnus Hagander

magnus@hagander.net

almost 7 years ago

In reply to: Michael Paquier (#4)

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

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

Michael Paquier

michael@paquier.xyz

almost 7 years ago

In reply to: Magnus Hagander (#9)

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

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

LGTM.

Okay, committed.
--
Michael