On 2020-02-03 13:47, Andres Freund wrote:
Comment:
- It'd be good to split out the feature independent refactorings, like
the introduction of InitControlFile(), into their own commit. Right
now it's hard to separate out what should just should be moved code,
and what should be behavioural changes. Especially when there's stuff
like just reindenting comments as part of the patch.
Agreed. Here are three refactoring patches extracted that seem useful
on their own.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 49b51b362b4d86103c74057186154a42b9ef335b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 17 Feb 2020 17:35:48 +0100
Subject: [PATCH 1/3] pg_resetwal: Rename function to avoid potential conflict
ReadControlFile() here conflicts with a function of the same name in
xlog.c. There is no actual conflict right now, but since
pg_resetwal.c reaches deep inside backend headers, it's possible in
the future.
---
src/bin/pg_resetwal/pg_resetwal.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_resetwal/pg_resetwal.c
b/src/bin/pg_resetwal/pg_resetwal.c
index f9cfeae264..c9edeb54d3 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -76,7 +76,7 @@ static int WalSegSz;
static int set_wal_segsize;
static void CheckDataVersion(void);
-static bool ReadControlFile(void);
+static bool read_controlfile(void);
static void GuessControlValues(void);
static void PrintControlValues(bool guessed);
static void PrintNewControlValues(void);
@@ -393,7 +393,7 @@ main(int argc, char *argv[])
/*
* Attempt to read the existing pg_control file
*/
- if (!ReadControlFile())
+ if (!read_controlfile())
GuessControlValues();
/*
@@ -578,7 +578,7 @@ CheckDataVersion(void)
* to the current format. (Currently we don't do anything of the sort.)
*/
static bool
-ReadControlFile(void)
+read_controlfile(void)
{
int fd;
int len;
--
2.25.0
From 91c816533a9e79623576a9303b2a793ee713eaed Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 17 Feb 2020 17:46:37 +0100
Subject: [PATCH 2/3] Reformat code comment
---
src/backend/access/transam/xlog.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 3813eadfb4..b017fd286f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6297,16 +6297,17 @@ StartupXLOG(void)
/*----------
* If we previously crashed, perform a couple of actions:
- * - The pg_wal directory may still include some temporary WAL
segments
- * used when creating a new segment, so perform some clean up to not
- * bloat this path. This is done first as there is no point to sync
this
- * temporary data.
- * - There might be data which we had written, intending to fsync
it,
- * but which we had not actually fsync'd yet. Therefore, a power failure
- * in the near future might cause earlier unflushed writes to be lost,
- * even though more recent data written to disk from here on would be
- * persisted. To avoid that, fsync the entire data directory.
- *---------
+ *
+ * - The pg_wal directory may still include some temporary WAL segments
+ * used when creating a new segment, so perform some clean up to not
+ * bloat this path. This is done first as there is no point to sync
+ * this temporary data.
+ *
+ * - There might be data which we had written, intending to fsync it,
but
+ * which we had not actually fsync'd yet. Therefore, a power failure
in
+ * the near future might cause earlier unflushed writes to be lost,
even
+ * though more recent data written to disk from here on would be
+ * persisted. To avoid that, fsync the entire data directory.
*/
if (ControlFile->state != DB_SHUTDOWNED &&
ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
--
2.25.0
From 0cebb19f719f1894140f13739f4a1851a929186d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 17 Feb 2020 17:58:02 +0100
Subject: [PATCH 3/3] Factor out InitControlFile() from BootStrapXLOG()
Right now this only makes BootStrapXLOG() a bit more manageable, but
in the future there may be external callers.
---
src/backend/access/transam/xlog.c | 70 +++++++++++++++++--------------
1 file changed, 39 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index b017fd286f..fd527f20c5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -903,6 +903,7 @@ static void CheckRecoveryConsistency(void);
static XLogRecord *ReadCheckpointRecord(XLogReaderState *xlogreader,
XLogRecPtr RecPtr, int whichChkpt, bool report);
static bool rescanLatestTimeLine(void);
+static void InitControlFile(uint64 sysidentifier);
static void WriteControlFile(void);
static void ReadControlFile(void);
static char *str_time(pg_time_t tnow);
@@ -4486,12 +4487,49 @@ rescanLatestTimeLine(void)
* given a preloaded buffer, ReadControlFile() loads the buffer from
* the pg_control file (during postmaster or standalone-backend startup),
* and UpdateControlFile() rewrites pg_control after we modify xlog state.
+ * InitControlFile() fills the buffer with initial values.
*
* For simplicity, WriteControlFile() initializes the fields of pg_control
* that are related to checking backend/database compatibility, and
* ReadControlFile() verifies they are correct. We could split out the
* I/O and compatibility-check functions, but there seems no need currently.
*/
+
+static void
+InitControlFile(uint64 sysidentifier)
+{
+ char mock_auth_nonce[MOCK_AUTH_NONCE_LEN];
+
+ /*
+ * Generate a random nonce. This is used for authentication requests
that
+ * will fail because the user does not exist. The nonce is used to
create
+ * a genuine-looking password challenge for the non-existent user, in
lieu
+ * of an actual stored password.
+ */
+ if (!pg_strong_random(mock_auth_nonce, MOCK_AUTH_NONCE_LEN))
+ ereport(PANIC,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate secret
authorization token")));
+
+ memset(ControlFile, 0, sizeof(ControlFileData));
+ /* Initialize pg_control status fields */
+ ControlFile->system_identifier = sysidentifier;
+ memcpy(ControlFile->mock_authentication_nonce, mock_auth_nonce,
MOCK_AUTH_NONCE_LEN);
+ ControlFile->state = DB_SHUTDOWNED;
+ ControlFile->unloggedLSN = FirstNormalUnloggedLSN;
+
+ /* Set important parameter values for use when replaying WAL */
+ ControlFile->MaxConnections = MaxConnections;
+ ControlFile->max_worker_processes = max_worker_processes;
+ ControlFile->max_wal_senders = max_wal_senders;
+ ControlFile->max_prepared_xacts = max_prepared_xacts;
+ ControlFile->max_locks_per_xact = max_locks_per_xact;
+ ControlFile->wal_level = wal_level;
+ ControlFile->wal_log_hints = wal_log_hints;
+ ControlFile->track_commit_timestamp = track_commit_timestamp;
+ ControlFile->data_checksum_version = bootstrap_data_checksum_version;
+}
+
static void
WriteControlFile(void)
{
@@ -5088,7 +5126,6 @@ BootStrapXLOG(void)
char *recptr;
bool use_existent;
uint64 sysidentifier;
- char mock_auth_nonce[MOCK_AUTH_NONCE_LEN];
struct timeval tv;
pg_crc32c crc;
@@ -5109,17 +5146,6 @@ BootStrapXLOG(void)
sysidentifier |= ((uint64) tv.tv_usec) << 12;
sysidentifier |= getpid() & 0xFFF;
- /*
- * Generate a random nonce. This is used for authentication requests
that
- * will fail because the user does not exist. The nonce is used to
create
- * a genuine-looking password challenge for the non-existent user, in
lieu
- * of an actual stored password.
- */
- if (!pg_strong_random(mock_auth_nonce, MOCK_AUTH_NONCE_LEN))
- ereport(PANIC,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate secret
authorization token")));
-
/* First timeline ID is always 1 */
ThisTimeLineID = 1;
@@ -5227,30 +5253,12 @@ BootStrapXLOG(void)
openLogFile = -1;
/* Now create pg_control */
-
- memset(ControlFile, 0, sizeof(ControlFileData));
- /* Initialize pg_control status fields */
- ControlFile->system_identifier = sysidentifier;
- memcpy(ControlFile->mock_authentication_nonce, mock_auth_nonce,
MOCK_AUTH_NONCE_LEN);
- ControlFile->state = DB_SHUTDOWNED;
+ InitControlFile(sysidentifier);
ControlFile->time = checkPoint.time;
ControlFile->checkPoint = checkPoint.redo;
ControlFile->checkPointCopy = checkPoint;
- ControlFile->unloggedLSN = FirstNormalUnloggedLSN;
-
- /* Set important parameter values for use when replaying WAL */
- ControlFile->MaxConnections = MaxConnections;
- ControlFile->max_worker_processes = max_worker_processes;
- ControlFile->max_wal_senders = max_wal_senders;
- ControlFile->max_prepared_xacts = max_prepared_xacts;
- ControlFile->max_locks_per_xact = max_locks_per_xact;
- ControlFile->wal_level = wal_level;
- ControlFile->wal_log_hints = wal_log_hints;
- ControlFile->track_commit_timestamp = track_commit_timestamp;
- ControlFile->data_checksum_version = bootstrap_data_checksum_version;
/* some additional ControlFile fields are set in WriteControlFile() */
-
WriteControlFile();
/* Bootstrap the commit log, too */
--
2.25.0
