pgsql: pg_ctl: Detect current standby state from pg_control
pg_ctl: Detect current standby state from pg_control
pg_ctl used to determine whether a server was in standby mode by looking
for a recovery.conf file. With this change, it instead looks into
pg_control, which is potentially more accurate. There are also
occasional discussions about removing recovery.conf, so this removes one
dependency.
Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/c1dc51d4844e2a37412b034c07c1c5a439ba0b9d
Modified Files
--------------
src/backend/utils/misc/pg_controldata.c | 12 +++++++++
src/bin/pg_controldata/pg_controldata.c | 4 +++
src/bin/pg_ctl/pg_ctl.c | 47 ++++++++++++++++++++++++++-------
src/common/controldata_utils.c | 15 +++++------
src/include/common/controldata_utils.h | 2 +-
5 files changed, 62 insertions(+), 18 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Peter Eisentraut <peter_e@gmx.net> writes:
pg_ctl: Detect current standby state from pg_control
Coverity thinks that this patch introduced a bunch of
null-pointer-dereference hazards, and AFAICS it is right.
The change in get_controlfile()'s API is completely broken
and needs to be undone.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Mon, Sep 26, 2016 at 12:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
pg_ctl: Detect current standby state from pg_control
Coverity thinks that this patch introduced a bunch of
null-pointer-dereference hazards, and AFAICS it is right.
The change in get_controlfile()'s API is completely broken
and needs to be undone.
So you'd rather have a sleep(1) and complicate this code to replace it
with a WaitLatch() when the code is used by the backend? I don't agree
with that as the new API is cleaner as presented, though I agree that
it is a change that caller does not get any result in case of a CRC
mismatch, but who's going to use results that cannot be trusted
anyway? It seems to me that the correct fix here is that
pg_controldata.c should just exit like in the attached patch. Somewhat
I missed that during my lookup of c1dc51d4.
If we would still want callers of get_controlfile() to be allowed to
read untrustworthy results, we could just add a boolean status flag..
--
Michael
Attachments:
controldata-untrust.patchapplication/x-download; name=controldata-untrust.patchDownload
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e92feab..82a58d5 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -157,9 +157,12 @@ main(int argc, char *argv[])
/* get a copy of the control file */
ControlFile = get_controlfile(DataDir, progname);
if (!ControlFile)
+ {
printf(_("WARNING: Calculated CRC checksum does not match value stored in file.\n"
"Either the file is corrupt, or it has a different layout than this program\n"
- "is expecting. The results below are untrustworthy.\n\n"));
+ "is expecting.\n"));
+ exit(1);
+ }
/*
* This slightly-chintzy coding will work as long as the control file
Michael Paquier <michael.paquier@gmail.com> writes:
On Mon, Sep 26, 2016 at 12:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Coverity thinks that this patch introduced a bunch of
null-pointer-dereference hazards, and AFAICS it is right.
The change in get_controlfile()'s API is completely broken
and needs to be undone.
So you'd rather have a sleep(1) and complicate this code to replace it
with a WaitLatch() when the code is used by the backend? I don't agree
with that as the new API is cleaner as presented, though I agree that
it is a change that caller does not get any result in case of a CRC
mismatch, but who's going to use results that cannot be trusted
anyway?
Well, they certainly won't be using any untrustworthy reports if
pg_controldata dumps core instead of printing the data, which is what
will happen in HEAD. Although I don't understand your reference to
needing a sleep. I think that it's 100% pointless for get_control_dbstate
to be worried about transient CRC failures. If writes to pg_control
aren't atomic then we have problems enormously larger than whether
"pg_ctl promote" throws an error or not.
It seems to me that the correct fix here is that
pg_controldata.c should just exit like in the attached patch.
No. Removing pg_controldata's designed-in ability to print information
from corrupted pg_control files was not part of the advertised impact of
this patch, and I will absolutely not hold still for it happening as an
accidental consequence of ill-advised refactoring.
It may well be that you need two different APIs for get_controlfile(),
maybe with a flag, or two different wrappers around some common code.
Or just go back to using #ifdef FRONTEND, and forget the silliness of
expecting pg_ctl not to throw an error for bad pg_control content.
BTW, now that get_controlfile has become a global symbol rather than
something private to pg_controldata.c, I suggest that it needs a less
generic name. "control file" could apply to a lot of things.
Possibly "get_pg_control_contents" would be better.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On 9/26/16 8:53 AM, Tom Lane wrote:
I think that it's 100% pointless for get_control_dbstate
to be worried about transient CRC failures. If writes to pg_control
aren't atomic then we have problems enormously larger than whether
"pg_ctl promote" throws an error or not.
The new code was designed to handle that, but if we think it's not an
issue, then we can simplify it. I'm on it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On 9/26/16 7:56 PM, Peter Eisentraut wrote:
On 9/26/16 8:53 AM, Tom Lane wrote:
I think that it's 100% pointless for get_control_dbstate
to be worried about transient CRC failures. If writes to pg_control
aren't atomic then we have problems enormously larger than whether
"pg_ctl promote" throws an error or not.The new code was designed to handle that, but if we think it's not an
issue, then we can simplify it. I'm on it.
How about this?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-CRC-check-handling-in-get_controlfile.patchinvalid/octet-stream; name=0001-Fix-CRC-check-handling-in-get_controlfile.patchDownload
From 12665d0926aca46e63ce3a0e6be1c5da7e470091 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Mon, 26 Sep 2016 12:00:00 -0400
Subject: [PATCH] Fix CRC check handling in get_controlfile
---
src/backend/utils/misc/pg_controldata.c | 8 ++++----
src/bin/pg_controldata/pg_controldata.c | 5 +++--
src/bin/pg_ctl/pg_ctl.c | 23 ++++++-----------------
src/common/controldata_utils.c | 22 +++++++++++++++++-----
src/include/common/controldata_utils.h | 2 +-
5 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c
index 4f9de83..2ba505c 100644
--- a/src/backend/utils/misc/pg_controldata.c
+++ b/src/backend/utils/misc/pg_controldata.c
@@ -51,7 +51,7 @@ pg_control_system(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* read the control file */
- ControlFile = get_controlfile(DataDir, NULL);
+ ControlFile = get_controlfile(DataDir, NULL, NULL);
if (!ControlFile)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
@@ -130,7 +130,7 @@ pg_control_checkpoint(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* Read the control file. */
- ControlFile = get_controlfile(DataDir, NULL);
+ ControlFile = get_controlfile(DataDir, NULL, NULL);
if (!ControlFile)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
@@ -235,7 +235,7 @@ pg_control_recovery(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* read the control file */
- ControlFile = get_controlfile(DataDir, NULL);
+ ControlFile = get_controlfile(DataDir, NULL, NULL);
if (!ControlFile)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
@@ -303,7 +303,7 @@ pg_control_init(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* read the control file */
- ControlFile = get_controlfile(DataDir, NULL);
+ ControlFile = get_controlfile(DataDir, NULL, NULL);
if (!ControlFile)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e92feab..20077a6 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -86,6 +86,7 @@ int
main(int argc, char *argv[])
{
ControlFileData *ControlFile;
+ bool crc_ok;
char *DataDir = NULL;
time_t time_tmp;
char pgctime_str[128];
@@ -155,8 +156,8 @@ main(int argc, char *argv[])
}
/* get a copy of the control file */
- ControlFile = get_controlfile(DataDir, progname);
- if (!ControlFile)
+ ControlFile = get_controlfile(DataDir, progname, &crc_ok);
+ if (!crc_ok)
printf(_("WARNING: Calculated CRC checksum does not match value stored in file.\n"
"Either the file is corrupt, or it has a different layout than this program\n"
"is expecting. The results below are untrustworthy.\n\n"));
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 052b02e..7400a54 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2147,28 +2147,17 @@ static DBState
get_control_dbstate(void)
{
DBState ret;
+ ControlFileData *control_file_data = get_controlfile(pg_data, progname, NULL);
- for (;;)
+ if (!control_file_data)
{
- ControlFileData *control_file_data = get_controlfile(pg_data, progname);
-
- if (control_file_data)
- {
- ret = control_file_data->state;
- pfree(control_file_data);
- return ret;
- }
-
- if (wait_seconds > 0)
- {
- pg_usleep(1000000); /* 1 sec */
- wait_seconds--;
- continue;
- }
-
write_stderr(_("%s: control file appears to be corrupt\n"), progname);
exit(1);
}
+
+ ret = control_file_data->state;
+ pfree(control_file_data);
+ return ret;
}
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index f218d25..a24ef8b 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -29,15 +29,17 @@
#include "port/pg_crc32c.h"
/*
- * get_controlfile(char *DataDir, const char *progname)
+ * get_controlfile(char *DataDir, const char *progname, bool *crc_ok_p)
*
* Get controlfile values. The caller is responsible
* for pfreeing the result.
*
- * Returns NULL if the CRC did not match.
+ * If crc_ok_p is not NULL, the result of the CRC check will be returned there
+ * and the actual control file data will be returned either way. If crc_ok_p
+ * is NULL and the CRC check fails, the return value is NULL.
*/
ControlFileData *
-get_controlfile(const char *DataDir, const char *progname)
+get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p)
{
ControlFileData *ControlFile;
int fd;
@@ -85,8 +87,18 @@ get_controlfile(const char *DataDir, const char *progname)
if (!EQ_CRC32C(crc, ControlFile->crc))
{
- pfree(ControlFile);
- return NULL;
+ if (crc_ok_p)
+ *crc_ok_p = false;
+ else
+ {
+ pfree(ControlFile);
+ return NULL;
+ }
+ }
+ else
+ {
+ if (crc_ok_p)
+ *crc_ok_p = true;
}
/* Make sure the control file is valid byte order. */
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index f834624..a822cba 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/include/common/controldata_utils.h
@@ -12,6 +12,6 @@
#include "catalog/pg_control.h"
-extern ControlFileData *get_controlfile(const char *DataDir, const char *progname);
+extern ControlFileData *get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p);
#endif /* COMMON_CONTROLDATA_UTILS_H */
--
2.10.0
On Tue, Sep 27, 2016 at 9:45 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 9/26/16 7:56 PM, Peter Eisentraut wrote:
On 9/26/16 8:53 AM, Tom Lane wrote:
I think that it's 100% pointless for get_control_dbstate
to be worried about transient CRC failures. If writes to pg_control
aren't atomic then we have problems enormously larger than whether
"pg_ctl promote" throws an error or not.The new code was designed to handle that, but if we think it's not an
issue, then we can simplify it. I'm on it.How about this?
+ if (crc_ok_p)
+ *crc_ok_p = false;
+ else
+ {
+ pfree(ControlFile);
+ return NULL;
+ }
Seems overcomplicated to me. How about returning the control file all
the time and let the caller pfree the result? You could then use
crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.Also, if the sleep() loop is removed for promote -w, we assume that
we'd fail immediately in case of a corrupted control file. Could it be
possible that after a couple of seconds the backend gets it right and
overwrites a correct control file on top of a corrupted one? I am just
curious about such possibilities, still I am +1 for removing the
pg_usleep loop and failing right away as you propose.
If we do the renaming of get_controlfile(), it may be also a good idea
to do the same for get_configdata() in config_info.c for consistency.
That's not really critical IMHO though.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Seems overcomplicated to me. How about returning the control file all
the time and let the caller pfree the result? You could then use
crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.
In short I would just go with the attached and call it a day.
--
Michael
Attachments:
control-crc-checks-v2.patchapplication/x-patch; name=control-crc-checks-v2.patchDownload
diff --git a/src/backend/utils/misc/pg_controldata.c b/src/backend/utils/misc/pg_controldata.c
index 4f9de83..0c67833 100644
--- a/src/backend/utils/misc/pg_controldata.c
+++ b/src/backend/utils/misc/pg_controldata.c
@@ -34,6 +34,7 @@ pg_control_system(PG_FUNCTION_ARGS)
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
+ bool crc_ok;
/*
* Construct a tuple descriptor for the result row. This must match this
@@ -51,8 +52,8 @@ pg_control_system(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* read the control file */
- ControlFile = get_controlfile(DataDir, NULL);
- if (!ControlFile)
+ ControlFile = get_controlfile(DataDir, NULL, &crc_ok);
+ if (!crc_ok)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
@@ -83,6 +84,7 @@ pg_control_checkpoint(PG_FUNCTION_ARGS)
ControlFileData *ControlFile;
XLogSegNo segno;
char xlogfilename[MAXFNAMELEN];
+ bool crc_ok;
/*
* Construct a tuple descriptor for the result row. This must match this
@@ -130,8 +132,8 @@ pg_control_checkpoint(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* Read the control file. */
- ControlFile = get_controlfile(DataDir, NULL);
- if (!ControlFile)
+ ControlFile = get_controlfile(DataDir, NULL, &crc_ok);
+ if (!crc_ok)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
@@ -216,6 +218,7 @@ pg_control_recovery(PG_FUNCTION_ARGS)
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
+ bool crc_ok;
/*
* Construct a tuple descriptor for the result row. This must match this
@@ -235,8 +238,8 @@ pg_control_recovery(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* read the control file */
- ControlFile = get_controlfile(DataDir, NULL);
- if (!ControlFile)
+ ControlFile = get_controlfile(DataDir, NULL, &crc_ok);
+ if (!crc_ok)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
@@ -268,6 +271,7 @@ pg_control_init(PG_FUNCTION_ARGS)
TupleDesc tupdesc;
HeapTuple htup;
ControlFileData *ControlFile;
+ bool crc_ok;
/*
* Construct a tuple descriptor for the result row. This must match this
@@ -303,8 +307,8 @@ pg_control_init(PG_FUNCTION_ARGS)
tupdesc = BlessTupleDesc(tupdesc);
/* read the control file */
- ControlFile = get_controlfile(DataDir, NULL);
- if (!ControlFile)
+ ControlFile = get_controlfile(DataDir, NULL, &crc_ok);
+ if (!crc_ok)
ereport(ERROR,
(errmsg("calculated CRC checksum does not match value stored in file")));
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e92feab..20077a6 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -86,6 +86,7 @@ int
main(int argc, char *argv[])
{
ControlFileData *ControlFile;
+ bool crc_ok;
char *DataDir = NULL;
time_t time_tmp;
char pgctime_str[128];
@@ -155,8 +156,8 @@ main(int argc, char *argv[])
}
/* get a copy of the control file */
- ControlFile = get_controlfile(DataDir, progname);
- if (!ControlFile)
+ ControlFile = get_controlfile(DataDir, progname, &crc_ok);
+ if (!crc_ok)
printf(_("WARNING: Calculated CRC checksum does not match value stored in file.\n"
"Either the file is corrupt, or it has a different layout than this program\n"
"is expecting. The results below are untrustworthy.\n\n"));
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 052b02e..ab10d2f 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2147,28 +2147,18 @@ static DBState
get_control_dbstate(void)
{
DBState ret;
+ bool crc_ok;
+ ControlFileData *control_file_data = get_controlfile(pg_data, progname, &crc_ok);
- for (;;)
+ if (!crc_ok)
{
- ControlFileData *control_file_data = get_controlfile(pg_data, progname);
-
- if (control_file_data)
- {
- ret = control_file_data->state;
- pfree(control_file_data);
- return ret;
- }
-
- if (wait_seconds > 0)
- {
- pg_usleep(1000000); /* 1 sec */
- wait_seconds--;
- continue;
- }
-
write_stderr(_("%s: control file appears to be corrupt\n"), progname);
exit(1);
}
+
+ ret = control_file_data->state;
+ pfree(control_file_data);
+ return ret;
}
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index f218d25..822fda7 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -29,21 +29,24 @@
#include "port/pg_crc32c.h"
/*
- * get_controlfile(char *DataDir, const char *progname)
+ * get_controlfile(char *DataDir, const char *progname, bool *crc_ok_p)
*
- * Get controlfile values. The caller is responsible
- * for pfreeing the result.
+ * Get controlfile values. The result is returned as a palloc'ed copy
+ * of the control file data.
*
- * Returns NULL if the CRC did not match.
+ * crc_ok_p can be used by the caller for sanity checks to see if the
+ * CRC of the control file data is in a consistent or not.
*/
ControlFileData *
-get_controlfile(const char *DataDir, const char *progname)
+get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p)
{
ControlFileData *ControlFile;
int fd;
char ControlFilePath[MAXPGPATH];
pg_crc32c crc;
+ Assert(crc_ok_p != NULL);
+
ControlFile = palloc(sizeof(ControlFileData));
snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
@@ -83,11 +86,7 @@ get_controlfile(const char *DataDir, const char *progname)
offsetof(ControlFileData, crc));
FIN_CRC32C(crc);
- if (!EQ_CRC32C(crc, ControlFile->crc))
- {
- pfree(ControlFile);
- return NULL;
- }
+ *crc_ok_p = EQ_CRC32C(crc, ControlFile->crc);
/* Make sure the control file is valid byte order. */
if (ControlFile->pg_control_version % 65536 == 0 &&
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index f834624..a822cba 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/include/common/controldata_utils.h
@@ -12,6 +12,6 @@
#include "catalog/pg_control.h"
-extern ControlFileData *get_controlfile(const char *DataDir, const char *progname);
+extern ControlFileData *get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p);
#endif /* COMMON_CONTROLDATA_UTILS_H */
On 9/28/16 12:44 AM, Michael Paquier wrote:
On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:Seems overcomplicated to me. How about returning the control file all
the time and let the caller pfree the result? You could then use
crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.In short I would just go with the attached and call it a day.
Pushed that way. Thanks!
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers