Re: [HACKERS] [COMMITTERS] pgsql: pg_ctl: Detect current standby state from pg_control

[Peter Eisentraut]

On 9/28/16 12:44 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
>  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

Re: [HACKERS] [COMMITTERS] pgsql: pg_ctl: Detect current standby state from pg_control

[Michael Paquier]

On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
 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
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, _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, _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, _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, _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, _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, _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(100);		/* 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 

Re: [HACKERS] [COMMITTERS] pgsql: pg_ctl: Detect current standby state from pg_control

[Michael Paquier]

On Tue, Sep 27, 2016 at 9:45 AM, Peter Eisentraut
 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