Small review comment on pg_checksums
Hi,
While reading the pg_checksums code I found the following comment
"Check if cluster is running" is not placed at right place.
/* Check if cluster is running */
ControlFile = get_controlfile(DataDir, &crc_ok);
if (!crc_ok)
{
pg_log_error("pg_control CRC value is incorrect");
exit(1);
}
if (ControlFile->pg_control_version != PG_CONTROL_VERSION)
{
pg_log_error("cluster is not compatible with this version of
pg_checksums");
exit(1);
}
if (ControlFile->blcksz != BLCKSZ)
{
pg_log_error("database cluster is not compatible");
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)
{
pg_log_error("cluster must be shut down");
exit(1);
}
So I'd like to propose a small fix for that; move the comment to the
right place and add another comment. Please find an attached small
patch.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
fix_pg_checksums.patchapplication/octet-stream; name=fix_pg_checksums.patchDownload
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 1d75aa0..faf8805 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -475,7 +475,7 @@ main(int argc, char *argv[])
exit(1);
}
- /* Check if cluster is running */
+ /* Read the control file and check CRC and incompatibility */
ControlFile = get_controlfile(DataDir, &crc_ok);
if (!crc_ok)
{
@@ -497,6 +497,7 @@ main(int argc, char *argv[])
exit(1);
}
+ /* Check if cluster is running */
if (ControlFile->state != DB_SHUTDOWNED &&
ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
{
On Thu, Jun 06, 2019 at 05:16:30PM +0900, Masahiko Sawada wrote:
So I'd like to propose a small fix for that; move the comment to the
right place and add another comment. Please find an attached small
patch.
No objections to that. Perhaps we should also mention that this does
not protect from someone starting the cluster concurrently and that
the reason why we require a clean shutdown is that we may get checksum
failures because of torn pages?
--
Michael
On Thu, Jun 6, 2019 at 10:21 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Jun 06, 2019 at 05:16:30PM +0900, Masahiko Sawada wrote:
So I'd like to propose a small fix for that; move the comment to the
right place and add another comment. Please find an attached small
patch.No objections to that. Perhaps we should also mention that this does
not protect from someone starting the cluster concurrently and that
the reason why we require a clean shutdown is that we may get checksum
failures because of torn pages?
Agreed. Please find an attached patch.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
fix_pg_checksums_v2.patchapplication/octet-stream; name=fix_pg_checksums_v2.patchDownload
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 7c7a75c..07e6fb6 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -475,7 +475,7 @@ main(int argc, char *argv[])
exit(1);
}
- /* Check if cluster is running */
+ /* Read the control file and check CRC and incompatibility */
ControlFile = get_controlfile(DataDir, &crc_ok);
if (!crc_ok)
{
@@ -497,6 +497,11 @@ main(int argc, char *argv[])
exit(1);
}
+ /*
+ * Check if cluster is running. The reason why we require a clean shutdown
+ * is that we may get checksum failures because of torn pages. Note that
+ * it does not protect someone starting the cluster concurrently.
+ */
if (ControlFile->state != DB_SHUTDOWNED &&
ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
{
On Fri, Jun 07, 2019 at 03:30:35PM +0900, Masahiko Sawada wrote:
Agreed. Please find an attached patch.
Thanks, committed.
--
Michael
On Fri, Jun 7, 2019 at 8:52 PM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jun 07, 2019 at 03:30:35PM +0900, Masahiko Sawada wrote:
Agreed. Please find an attached patch.
Thanks, committed.
Thank you!
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center