Deduplicate code updating ControleFile's DBState.
Hi,
I would like to propose a patch that removes the duplicate code
setting database state in the control file.
The patch is straightforward but the only concern is that in
StartupXLOG(), SharedRecoveryState now gets updated only with spin
lock; earlier it also had ControlFileLock in addition to that. AFAICU,
I don't see any problem there, since until the startup process exists
other backends could not connect and write a WAL record.
Regards,
Amul Sul.
EDB: http://www.enterprisedb.com
Attachments:
v1-0001-Deduplicate-code-updating-ControleFile-s-DBState-.patchapplication/x-patch; name=v1-0001-Deduplicate-code-updating-ControleFile-s-DBState-.patchDownload
From 4728be13bc17183f9869b1c040d5c72d2969e736 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Tue, 14 Sep 2021 01:30:54 -0400
Subject: [PATCH v1] Deduplicate code updating ControleFile's DBState only.
---
src/backend/access/transam/xlog.c | 53 +++++++++++++------------------
src/include/access/xlog.h | 3 ++
2 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e51a7a749da..381a3576d5e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4951,6 +4951,19 @@ UpdateControlFile(void)
update_controlfile(DataDir, ControlFile, true);
}
+/*
+ * Useful to set only ControlFile's database state.
+ */
+void
+SetControlFileDBState(DBState state)
+{
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+ ControlFile->state = state;
+ ControlFile->time = (pg_time_t) time(NULL);
+ UpdateControlFile();
+ LWLockRelease(ControlFileLock);
+}
+
/*
* Returns the unique system identifier from control file.
*/
@@ -8072,29 +8085,18 @@ StartupXLOG(void)
/*
* All done with end-of-recovery actions.
*
- * Now allow backends to write WAL and update the control file status in
- * consequence. SharedRecoveryState, that controls if backends can write
- * WAL, is updated while holding ControlFileLock to prevent other backends
- * to look at an inconsistent state of the control file in shared memory.
- * There is still a small window during which backends can write WAL and
- * the control file is still referring to a system not in DB_IN_PRODUCTION
- * state while looking at the on-disk control file.
+ * Now allow backends to write WAL and update the control file status and
+ * SharedRecoveryState, that controls if backends can write WAL.
*
- * Also, we use info_lck to update SharedRecoveryState to ensure that
- * there are no race conditions concerning visibility of other recent
- * updates to shared memory.
+ * We use info_lck to update SharedRecoveryState to ensure that there are no
+ * race conditions concerning visibility of other recent updates to shared
+ * memory.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
-
+ SetControlFileDBState(DB_IN_PRODUCTION);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
SpinLockRelease(&XLogCtl->info_lck);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
-
/*
* If there were cascading standby servers connected to us, nudge any wal
* sender processes to notice that we've been promoted.
@@ -8952,13 +8954,7 @@ CreateCheckPoint(int flags)
START_CRIT_SECTION();
if (shutdown)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNING;
- ControlFile->time = (pg_time_t) time(NULL);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNING);
/*
* Let smgr prepare for checkpoint; this has to happen before we determine
@@ -9507,13 +9503,8 @@ CreateRestartPoint(int flags)
UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
if (flags & CHECKPOINT_IS_SHUTDOWN)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- ControlFile->time = (pg_time_t) time(NULL);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNED_IN_RECOVERY);
+
return false;
}
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 0a8ede700de..3a485d86647 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -253,6 +253,8 @@ typedef enum WALAvailability
WALAVAIL_REMOVED /* WAL segment has been removed */
} WALAvailability;
+/* Avoid header inclusion */
+enum DBState;
struct XLogRecData;
extern XLogRecPtr XLogInsertRecord(struct XLogRecData *rdata,
@@ -291,6 +293,7 @@ extern TimestampTz GetLatestXTime(void);
extern TimestampTz GetCurrentChunkReplayStartTime(void);
extern void UpdateControlFile(void);
+extern void SetControlFileDBState(enum DBState state);
extern uint64 GetSystemIdentifier(void);
extern char *GetMockAuthenticationNonce(void);
extern bool DataChecksumsEnabled(void);
--
2.18.0
On 9/13/21, 11:06 PM, "Amul Sul" <sulamul@gmail.com> wrote:
The patch is straightforward but the only concern is that in
StartupXLOG(), SharedRecoveryState now gets updated only with spin
lock; earlier it also had ControlFileLock in addition to that. AFAICU,
I don't see any problem there, since until the startup process exists
other backends could not connect and write a WAL record.
It looks like ebdf5bf intentionally made sure that we hold
ControlFileLock while updating SharedRecoveryInProgress (now
SharedRecoveryState after 4e87c48). The thread for this change [0]/messages/by-id/CAB7nPqTS5J3-G_zTow0Kc5oqZn877RDDN1Mfcqm2PscAS7FnAw@mail.gmail.com
has some additional details.
As far as the patch goes, I'm not sure why SetControlFileDBState()
needs to be exported, and TBH I don't know if this change is really a
worthwhile improvement. ISTM the main benefit is that it could help
avoid cases where we update the state but not the time. However,
there's still nothing preventing that, and I don't get the idea that
it was really a big problem to begin with.
Nathan
[0]: /messages/by-id/CAB7nPqTS5J3-G_zTow0Kc5oqZn877RDDN1Mfcqm2PscAS7FnAw@mail.gmail.com
On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 9/13/21, 11:06 PM, "Amul Sul" <sulamul@gmail.com> wrote:
The patch is straightforward but the only concern is that in
StartupXLOG(), SharedRecoveryState now gets updated only with spin
lock; earlier it also had ControlFileLock in addition to that. AFAICU,
I don't see any problem there, since until the startup process exists
other backends could not connect and write a WAL record.It looks like ebdf5bf intentionally made sure that we hold
ControlFileLock while updating SharedRecoveryInProgress (now
SharedRecoveryState after 4e87c48). The thread for this change [0]
has some additional details.
Yeah, I saw that and ebdf5bf main intention was to minimize the gap
between both of them which was quite big previously. The comments
added by the same commit also describe the case that backends can
write WAL and the control file is still referring not in
DB_IN_PRODUCTION and IIUC, which seems to be acceptable.
Then the question is what would be wrong if a process can see an
inconsistent shared memory view for a small window? Might be
wait-promoting might behave unexpectedly, that I have to test.
As far as the patch goes, I'm not sure why SetControlFileDBState()
needs to be exported, and TBH I don't know if this change is really a
worthwhile improvement. ISTM the main benefit is that it could help
avoid cases where we update the state but not the time. However,
there's still nothing preventing that, and I don't get the idea that
it was really a big problem to begin with.
Oh ok, I was working on a different patch[1] where I want to call this
function from checkpointer, but I agree exporting function is not in
the scope of this patch.
Regards,
Amul
1] /messages/by-id/CAAJ_b97KZzdJsffwRK7w0XU5HnXkcgKgTR69t8cOZztsyXjkQw@mail.gmail.com
On 9/15/21, 4:47 AM, "Amul Sul" <sulamul@gmail.com> wrote:
On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
It looks like ebdf5bf intentionally made sure that we hold
ControlFileLock while updating SharedRecoveryInProgress (now
SharedRecoveryState after 4e87c48). The thread for this change [0]
has some additional details.Yeah, I saw that and ebdf5bf main intention was to minimize the gap
between both of them which was quite big previously. The comments
added by the same commit also describe the case that backends can
write WAL and the control file is still referring not in
DB_IN_PRODUCTION and IIUC, which seems to be acceptable.
Then the question is what would be wrong if a process can see an
inconsistent shared memory view for a small window? Might be
wait-promoting might behave unexpectedly, that I have to test.
For your proposed change, I would either leave out this particular
call site or add a "WithLock" version of the function.
void
SetControlFileDBState(DBState state)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
SetControlFileDBStateWithLock(state);
LWLockRelease(ControlFileLock);
}
void
SetControlFileDBStateWithLock(DBState state)
{
Assert(LWLockHeldByMeInMode(ControlFileLock, LW_EXCLUSIVE));
ControlFile->state = state;
ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
}
As far as the patch goes, I'm not sure why SetControlFileDBState()
needs to be exported, and TBH I don't know if this change is really a
worthwhile improvement. ISTM the main benefit is that it could help
avoid cases where we update the state but not the time. However,
there's still nothing preventing that, and I don't get the idea that
it was really a big problem to begin with.Oh ok, I was working on a different patch[1] where I want to call this
function from checkpointer, but I agree exporting function is not in
the scope of this patch.
Ah, I was missing this context. Perhaps this should be included in
the patch set for the other thread, especially if it will need to be
exported.
Nathan
On Wed, Sep 15, 2021 at 10:49:39PM +0000, Bossart, Nathan wrote:
Ah, I was missing this context. Perhaps this should be included in
the patch set for the other thread, especially if it will need to be
exported.
This part of the patch is mentioned at the top of the thread:
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
-
+ SetControlFileDBState(DB_IN_PRODUCTION);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
SpinLockRelease(&XLogCtl->info_lck);There is an assumption in this code to update SharedRecoveryState
*while* holding ControlFileLock. For example, see the following
comment in xlog.c, ReadRecord():
/*
* We update SharedRecoveryState while holding the lock on
* ControlFileLock so both states are consistent in shared
* memory.
*/
--
Michael
On Thu, Sep 16, 2021 at 5:17 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Sep 15, 2021 at 10:49:39PM +0000, Bossart, Nathan wrote:
Ah, I was missing this context. Perhaps this should be included in
the patch set for the other thread, especially if it will need to be
exported.This part of the patch is mentioned at the top of the thread: - LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); - ControlFile->state = DB_IN_PRODUCTION; - ControlFile->time = (pg_time_t) time(NULL); - + SetControlFileDBState(DB_IN_PRODUCTION); SpinLockAcquire(&XLogCtl->info_lck); XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE; SpinLockRelease(&XLogCtl->info_lck);There is an assumption in this code to update SharedRecoveryState
*while* holding ControlFileLock. For example, see the following
comment in xlog.c, ReadRecord():
/*
* We update SharedRecoveryState while holding the lock on
* ControlFileLock so both states are consistent in shared
* memory.
*/
Ok, understood, let's do that update with ControlFileLock, thanks.
Regards,
Amul
On Thu, Sep 16, 2021 at 4:19 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 9/15/21, 4:47 AM, "Amul Sul" <sulamul@gmail.com> wrote:
On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
It looks like ebdf5bf intentionally made sure that we hold
ControlFileLock while updating SharedRecoveryInProgress (now
SharedRecoveryState after 4e87c48). The thread for this change [0]
has some additional details.Yeah, I saw that and ebdf5bf main intention was to minimize the gap
between both of them which was quite big previously. The comments
added by the same commit also describe the case that backends can
write WAL and the control file is still referring not in
DB_IN_PRODUCTION and IIUC, which seems to be acceptable.
Then the question is what would be wrong if a process can see an
inconsistent shared memory view for a small window? Might be
wait-promoting might behave unexpectedly, that I have to test.For your proposed change, I would either leave out this particular
call site or add a "WithLock" version of the function.void
SetControlFileDBState(DBState state)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
SetControlFileDBStateWithLock(state);
LWLockRelease(ControlFileLock);
}void
SetControlFileDBStateWithLock(DBState state)
{
Assert(LWLockHeldByMeInMode(ControlFileLock, LW_EXCLUSIVE));ControlFile->state = state;
ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
}
+1, since skipping ControlFileLock for the DBState update is not the
right thing, let's have two different functions as per your suggestion
-- did the same in the attached version, thanks.As far as the patch goes, I'm not sure why SetControlFileDBState()
needs to be exported, and TBH I don't know if this change is really a
worthwhile improvement. ISTM the main benefit is that it could help
avoid cases where we update the state but not the time. However,
there's still nothing preventing that, and I don't get the idea that
it was really a big problem to begin with.Oh ok, I was working on a different patch[1] where I want to call this
function from checkpointer, but I agree exporting function is not in
the scope of this patch.Ah, I was missing this context. Perhaps this should be included in
the patch set for the other thread, especially if it will need to be
exported.
Ok, reverted those changes in the attached version.
I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?
Regards,
Amul
Attachments:
v2-0001-Deduplicate-code-updating-ControleFile-s-DBState-.patchapplication/x-patch; name=v2-0001-Deduplicate-code-updating-ControleFile-s-DBState-.patchDownload
From 4aa18feb38ec7a77ebc8215ea4207071ce81d9d5 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Mon, 20 Sep 2021 01:50:59 -0400
Subject: [PATCH v2] Deduplicate code updating ControleFile's DBState only.
---
src/backend/access/transam/xlog.c | 48 ++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 17 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e51a7a749da..df1202e07da 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -931,6 +931,8 @@ static bool rescanLatestTimeLine(void);
static void InitControlFile(uint64 sysidentifier);
static void WriteControlFile(void);
static void ReadControlFile(void);
+static void SetControlFileDBStateWithLock(DBState state);
+static void SetControlFileDBState(DBState state);
static char *str_time(pg_time_t tnow);
static void SetPromoteIsTriggered(void);
static bool CheckForStandbyTrigger(void);
@@ -4951,6 +4953,31 @@ UpdateControlFile(void)
update_controlfile(DataDir, ControlFile, true);
}
+/*
+ * Useful to set only ControlFile's database state when a caller has already
+ * acquired exclusive ControlFileLock.
+ */
+static void
+SetControlFileDBStateWithLock(DBState state)
+{
+ Assert(LWLockHeldByMeInMode(ControlFileLock, LW_EXCLUSIVE));
+
+ ControlFile->state = state;
+ ControlFile->time = (pg_time_t) time(NULL);
+ UpdateControlFile();
+}
+
+/*
+ * Useful to set only ControlFile's database state.
+ */
+static void
+SetControlFileDBState(DBState state)
+{
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+ SetControlFileDBStateWithLock(state);
+ LWLockRelease(ControlFileLock);
+}
+
/*
* Returns the unique system identifier from control file.
*/
@@ -8085,14 +8112,12 @@ StartupXLOG(void)
* updates to shared memory.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
+ SetControlFileDBStateWithLock(DB_IN_PRODUCTION);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
SpinLockRelease(&XLogCtl->info_lck);
- UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -8952,13 +8977,7 @@ CreateCheckPoint(int flags)
START_CRIT_SECTION();
if (shutdown)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNING;
- ControlFile->time = (pg_time_t) time(NULL);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNING);
/*
* Let smgr prepare for checkpoint; this has to happen before we determine
@@ -9507,13 +9526,8 @@ CreateRestartPoint(int flags)
UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
if (flags & CHECKPOINT_IS_SHUTDOWN)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- ControlFile->time = (pg_time_t) time(NULL);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNED_IN_RECOVERY);
+
return false;
}
--
2.18.0
On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
+1, since skipping ControlFileLock for the DBState update is not the right thing, let's have two different functions as per your suggestion -- did the same in the attached version, thanks.
I see that the attached patch reorders the call to UpdateControlFile()
to before SharedRecoveryState is updated, which seems to go against
the intent of ebdf5bf. I'm not sure if this really creates that much
of a problem in practice, but it is a behavior change.
Also, I still think it might be better to include this patch in the
patch set where the exported function is needed. On its own, this is
a very small amount of refactoring that might not be totally
necessary.
I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?
I see a few places where UpdateControlFile() is called without
updating ControlFile->time. I haven't found any obvious reason for
that, so perhaps it would be okay to move it to update_controlfile().
Nathan
On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
+1, since skipping ControlFileLock for the DBState update is not the right thing, let's have two different functions as per your suggestion -- did the same in the attached version, thanks.I see that the attached patch reorders the call to UpdateControlFile()
to before SharedRecoveryState is updated, which seems to go against
the intent of ebdf5bf. I'm not sure if this really creates that much
of a problem in practice, but it is a behavior change.
I had to have a thought on the same and didn't see any problem and
test suits also fine but that doesn't mean the change is perfect, the
issue might be hard to reproduce if there are any. Let's see what
others think and for now, to be safe I have reverted this change.
Also, I still think it might be better to include this patch in the
patch set where the exported function is needed. On its own, this is
a very small amount of refactoring that might not be totally
necessary.
Well, the other patch set is quite big and complex. In my experience,
usually, people avoid downloading big sets due to lack of time and
such small refactoring patches usually don't get much detailed
attention.
Also, even though this patch is small, it is independent and has
nothing to do with other patch set whether it gets committed or not.
Still, proposing some improvement might not be a big one but nice to
have.
I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?I see a few places where UpdateControlFile() is called without
updating ControlFile->time. I haven't found any obvious reason for
that, so perhaps it would be okay to move it to update_controlfile().
Ok, thanks, did the same in the attached version.
Regards,
Amul Sul
Attachments:
v3-0001-Deduplicate-code-updating-ControleFile-s-DBState-.patchapplication/x-patch; name=v3-0001-Deduplicate-code-updating-ControleFile-s-DBState-.patchDownload
From c78d9abcd4a2856446d4afac240c2fcbdc0a315f Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Tue, 21 Sep 2021 00:52:55 -0400
Subject: [PATCH v3] Deduplicate code updating ControleFile's DBState and
timestamp
---
src/backend/access/transam/xlog.c | 40 +++++++++++++++----------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e51a7a749da..2d420334fbf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4942,15 +4942,28 @@ ReadControlFile(void)
}
/*
- * Utility wrapper to update the control file. Note that the control
- * file gets flushed.
+ * Utility wrapper to update the control file with update timestamp. Note that
+ * the control file gets flushed.
*/
void
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
+/*
+ * Set ControlFile's database state
+ */
+static void
+SetControlFileDBState(DBState state)
+{
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+ ControlFile->state = state;
+ UpdateControlFile();
+ LWLockRelease(ControlFileLock);
+}
+
/*
* Returns the unique system identifier from control file.
*/
@@ -7149,7 +7162,7 @@ StartupXLOG(void)
ControlFile->backupEndPoint = ControlFile->minRecoveryPoint;
}
}
- ControlFile->time = (pg_time_t) time(NULL);
+
/* No need to hold ControlFileLock yet, we aren't up far enough */
UpdateControlFile();
@@ -8086,7 +8099,6 @@ StartupXLOG(void)
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
@@ -8952,13 +8964,7 @@ CreateCheckPoint(int flags)
START_CRIT_SECTION();
if (shutdown)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNING;
- ControlFile->time = (pg_time_t) time(NULL);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNING);
/*
* Let smgr prepare for checkpoint; this has to happen before we determine
@@ -9226,7 +9232,6 @@ CreateCheckPoint(int flags)
ControlFile->state = DB_SHUTDOWNED;
ControlFile->checkPoint = ProcLastRecPtr;
ControlFile->checkPointCopy = checkPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/* crash recovery should always recover to the end of WAL */
ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
ControlFile->minRecoveryPointTLI = 0;
@@ -9354,7 +9359,6 @@ CreateEndOfRecoveryRecord(void)
* changes to this point.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->time = (pg_time_t) time(NULL);
ControlFile->minRecoveryPoint = recptr;
ControlFile->minRecoveryPointTLI = ThisTimeLineID;
UpdateControlFile();
@@ -9507,13 +9511,8 @@ CreateRestartPoint(int flags)
UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
if (flags & CHECKPOINT_IS_SHUTDOWN)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- ControlFile->time = (pg_time_t) time(NULL);
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNED_IN_RECOVERY);
+
return false;
}
@@ -9571,7 +9570,6 @@ CreateRestartPoint(int flags)
{
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/*
* Ensure minRecoveryPoint is past the checkpoint record. Normally,
--
2.18.0
On 9/20/21, 10:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?I see a few places where UpdateControlFile() is called without
updating ControlFile->time. I haven't found any obvious reason for
that, so perhaps it would be okay to move it to update_controlfile().Ok, thanks, did the same in the attached version.
void
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
Shouldn't we update the time in update_controlfile()? Also, can you
split this change into two patches (i.e., one for the timestamp change
and another for the refactoring)? Otherwise, this looks reasonable to
me.
Nathan
On Tue, Sep 21, 2021 at 9:43 PM Bossart, Nathan <bossartn@amazon.com> wrote:
On 9/20/21, 10:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?I see a few places where UpdateControlFile() is called without
updating ControlFile->time. I haven't found any obvious reason for
that, so perhaps it would be okay to move it to update_controlfile().Ok, thanks, did the same in the attached version.
void
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}Shouldn't we update the time in update_controlfile()?
If you see the callers of update_controlfile() except for
RewriteControlFile() no one else updates the timestamp before calling
it, I am not sure if that is intentional or not. That was the one
reason that was added in UpdateControlFile(). And another reason is
that if you look at all the deleting lines followed by
UpdateControlFile() & moving that to UpdateControlFile() wouldn't
change anything drastically.
IMO, anything going to change should update the timestamp as well,
that could be a bug then.
Also, can you
split this change into two patches (i.e., one for the timestamp change
and another for the refactoring)? Otherwise, this looks reasonable to
me.
Done, thanks for the review.
Regards,
Amul
Attachments:
v4-0002-Deduplicate-code-updating-ControleFile-s-DBState.patchapplication/octet-stream; name=v4-0002-Deduplicate-code-updating-ControleFile-s-DBState.patchDownload
From 4f77d6f349ad39d3e8074f18130622aaddd50069 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 23 Sep 2021 00:57:20 -0400
Subject: [PATCH v4 2/2] Deduplicate code updating ControleFile's DBState
---
src/backend/access/transam/xlog.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e439148aa7e..e865f23143a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4952,6 +4952,18 @@ UpdateControlFile(void)
update_controlfile(DataDir, ControlFile, true);
}
+/*
+ * Useful to set only ControlFile's database state.
+ */
+static void
+SetControlFileDBState(DBState state)
+{
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+ ControlFile->state = state;
+ UpdateControlFile();
+ LWLockRelease(ControlFileLock);
+}
+
/*
* Returns the unique system identifier from control file.
*/
@@ -8952,12 +8964,7 @@ CreateCheckPoint(int flags)
START_CRIT_SECTION();
if (shutdown)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNING;
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNING);
/*
* Let smgr prepare for checkpoint; this has to happen before we determine
@@ -9504,12 +9511,8 @@ CreateRestartPoint(int flags)
UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
if (flags & CHECKPOINT_IS_SHUTDOWN)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNED_IN_RECOVERY);
+
return false;
}
--
2.18.0
v4-0001-do-the-ControlFile-timestamp-setting-in-UpdateCon.patchapplication/octet-stream; name=v4-0001-do-the-ControlFile-timestamp-setting-in-UpdateCon.patchDownload
From 7f1c298e6a7fc1d2adf08a893ecb0c7951f4c038 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 23 Sep 2021 00:47:52 -0400
Subject: [PATCH v4 1/2] do the ControlFile timestamp setting in
UpdateControlFile()
---
src/backend/access/transam/xlog.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e51a7a749da..e439148aa7e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4942,12 +4942,13 @@ ReadControlFile(void)
}
/*
- * Utility wrapper to update the control file. Note that the control
- * file gets flushed.
+ * Utility wrapper to update the control file with update timestamp. Note that
+ * the control file gets flushed.
*/
void
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
@@ -7149,7 +7150,7 @@ StartupXLOG(void)
ControlFile->backupEndPoint = ControlFile->minRecoveryPoint;
}
}
- ControlFile->time = (pg_time_t) time(NULL);
+
/* No need to hold ControlFileLock yet, we aren't up far enough */
UpdateControlFile();
@@ -8086,7 +8087,6 @@ StartupXLOG(void)
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
@@ -8955,7 +8955,6 @@ CreateCheckPoint(int flags)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNING;
- ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
LWLockRelease(ControlFileLock);
}
@@ -9226,7 +9225,6 @@ CreateCheckPoint(int flags)
ControlFile->state = DB_SHUTDOWNED;
ControlFile->checkPoint = ProcLastRecPtr;
ControlFile->checkPointCopy = checkPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/* crash recovery should always recover to the end of WAL */
ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
ControlFile->minRecoveryPointTLI = 0;
@@ -9354,7 +9352,6 @@ CreateEndOfRecoveryRecord(void)
* changes to this point.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->time = (pg_time_t) time(NULL);
ControlFile->minRecoveryPoint = recptr;
ControlFile->minRecoveryPointTLI = ThisTimeLineID;
UpdateControlFile();
@@ -9510,7 +9507,6 @@ CreateRestartPoint(int flags)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
LWLockRelease(ControlFileLock);
}
@@ -9571,7 +9567,6 @@ CreateRestartPoint(int flags)
{
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/*
* Ensure minRecoveryPoint is past the checkpoint record. Normally,
--
2.18.0
On 9/22/21, 10:03 PM, "Amul Sul" <sulamul@gmail.com> wrote:
On Tue, Sep 21, 2021 at 9:43 PM Bossart, Nathan <bossartn@amazon.com> wrote:
Shouldn't we update the time in update_controlfile()?
If you see the callers of update_controlfile() except for
RewriteControlFile() no one else updates the timestamp before calling
it, I am not sure if that is intentional or not. That was the one
reason that was added in UpdateControlFile(). And another reason is
that if you look at all the deleting lines followed by
UpdateControlFile() & moving that to UpdateControlFile() wouldn't
change anything drastically.IMO, anything going to change should update the timestamp as well,
that could be a bug then.
I'm inclined to agree that anything that calls update_controlfile()
should update the timestamp. However, I wonder if the additional
calls to time() would have a noticeable impact.
Nathan
On Fri, Oct 01, 2021 at 05:47:45PM +0000, Bossart, Nathan wrote:
I'm inclined to agree that anything that calls update_controlfile()
should update the timestamp.
pg_control.h tells that:
pg_time_t time; /* time stamp of last pg_control update */
So, yes, that would be more consistent.
However, I wonder if the additional
calls to time() would have a noticeable impact.
I would not take that lightly either. Now, I don't think that any of
the code paths where UpdateControlFile() or update_controlfile() is
called are hot enough to worry about that.
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
I have to admit that it is a bit strange to do that in the backend but
not the frontend, so there is a good argument for doing that directly
in update_controlfile(). pg_resetwal does an update of the time, but
pg_rewind does not.
--
MichaelOn 10/1/21, 10:40 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
On Fri, Oct 01, 2021 at 05:47:45PM +0000, Bossart, Nathan wrote:
I'm inclined to agree that anything that calls update_controlfile()
should update the timestamp.pg_control.h tells that:
pg_time_t time; /* time stamp of last pg_control update */
So, yes, that would be more consistent.However, I wonder if the additional
calls to time() would have a noticeable impact.I would not take that lightly either. Now, I don't think that any of
the code paths where UpdateControlFile() or update_controlfile() is
called are hot enough to worry about that.UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
I have to admit that it is a bit strange to do that in the backend but
not the frontend, so there is a good argument for doing that directly
in update_controlfile(). pg_resetwal does an update of the time, but
pg_rewind does not.
I don't see any recent updates to this thread from Amul, so I'm
marking this one as waiting-for-author.
Nathan
On Thu, Nov 11, 2021 at 1:30 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 10/1/21, 10:40 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
On Fri, Oct 01, 2021 at 05:47:45PM +0000, Bossart, Nathan wrote:
I'm inclined to agree that anything that calls update_controlfile()
should update the timestamp.pg_control.h tells that:
pg_time_t time; /* time stamp of last pg_control update */
So, yes, that would be more consistent.However, I wonder if the additional
calls to time() would have a noticeable impact.I would not take that lightly either. Now, I don't think that any of
the code paths where UpdateControlFile() or update_controlfile() is
called are hot enough to worry about that.UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
I have to admit that it is a bit strange to do that in the backend but
not the frontend, so there is a good argument for doing that directly
in update_controlfile(). pg_resetwal does an update of the time, but
pg_rewind does not.
Thanks for the inputs -- moved timestamp setting inside update_controlfile().
I don't see any recent updates to this thread from Amul, so I'm
marking this one as waiting-for-author.
Sorry for the delay, please have a look at the attached version --
changing status to Needs review, thanks.
Regards,
Amul
Attachments:
v5-0001-Do-the-ControlFile-timestamp-setting-in-update_co.patchapplication/octet-stream; name=v5-0001-Do-the-ControlFile-timestamp-setting-in-update_co.patchDownload
From 463c19e5b577df88466bcc7867f605bc205837b7 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 23 Sep 2021 00:47:52 -0400
Subject: [PATCH v5 1/2] Do the ControlFile timestamp setting in
update_controlfile.
---
src/backend/access/transam/xlog.c | 9 +--------
src/bin/pg_resetwal/pg_resetwal.c | 2 --
src/common/controldata_utils.c | 4 ++++
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b54ec549705..669fa6b4d38 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5476,7 +5476,6 @@ BootStrapXLOG(void)
/* Now create pg_control */
InitControlFile(sysidentifier);
- ControlFile->time = checkPoint.time;
ControlFile->checkPoint = checkPoint.redo;
ControlFile->checkPointCopy = checkPoint;
@@ -7339,7 +7338,7 @@ StartupXLOG(void)
ControlFile->backupEndPoint = ControlFile->minRecoveryPoint;
}
}
- ControlFile->time = (pg_time_t) time(NULL);
+
/* No need to hold ControlFileLock yet, we aren't up far enough */
UpdateControlFile();
@@ -8199,7 +8198,6 @@ StartupXLOG(void)
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
@@ -9142,7 +9140,6 @@ CreateCheckPoint(int flags)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNING;
- ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
LWLockRelease(ControlFileLock);
}
@@ -9412,7 +9409,6 @@ CreateCheckPoint(int flags)
ControlFile->state = DB_SHUTDOWNED;
ControlFile->checkPoint = ProcLastRecPtr;
ControlFile->checkPointCopy = checkPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/* crash recovery should always recover to the end of WAL */
ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
ControlFile->minRecoveryPointTLI = 0;
@@ -9539,7 +9535,6 @@ CreateEndOfRecoveryRecord(void)
* changes to this point.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->time = (pg_time_t) time(NULL);
ControlFile->minRecoveryPoint = recptr;
ControlFile->minRecoveryPointTLI = xlrec.ThisTimeLineID;
UpdateControlFile();
@@ -9740,7 +9735,6 @@ CreateRestartPoint(int flags)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
LWLockRelease(ControlFileLock);
}
@@ -9801,7 +9795,6 @@ CreateRestartPoint(int flags)
{
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/*
* Ensure minRecoveryPoint is past the checkpoint record. Normally,
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 84b6f70ad6a..942c05d67f0 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -714,7 +714,6 @@ GuessControlValues(void)
ControlFile.checkPointCopy.oldestActiveXid = InvalidTransactionId;
ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
ControlFile.checkPoint = ControlFile.checkPointCopy.redo;
ControlFile.unloggedLSN = FirstNormalUnloggedLSN;
@@ -911,7 +910,6 @@ RewriteControlFile(void)
ControlFile.checkPointCopy.time = (pg_time_t) time(NULL);
ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
ControlFile.checkPoint = ControlFile.checkPointCopy.redo;
ControlFile.minRecoveryPoint = 0;
ControlFile.minRecoveryPointTLI = 0;
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 7d4af7881ea..4ce097c05ce 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -23,6 +23,7 @@
#include <unistd.h>
#include <sys/stat.h>
#include <fcntl.h>
+#include <time.h>
#include "access/xlog_internal.h"
#include "catalog/pg_control.h"
@@ -168,6 +169,9 @@ update_controlfile(const char *DataDir,
StaticAssertStmt(sizeof(ControlFileData) <= PG_CONTROL_FILE_SIZE,
"sizeof(ControlFileData) exceeds PG_CONTROL_FILE_SIZE");
+ /* Update timestamp */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/* Recalculate CRC of control file */
INIT_CRC32C(ControlFile->crc);
COMP_CRC32C(ControlFile->crc,
--
2.18.0
v5-0002-Deduplicate-code-updating-ControleFile-s-DBState.patchapplication/octet-stream; name=v5-0002-Deduplicate-code-updating-ControleFile-s-DBState.patchDownload
From 58cd27f85a300dc802e57d1370d4d99bb91e60be Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 23 Sep 2021 00:57:20 -0400
Subject: [PATCH v5 2/2] Deduplicate code updating ControleFile's DBState
---
src/backend/access/transam/xlog.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 669fa6b4d38..093946e48f5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5043,6 +5043,18 @@ UpdateControlFile(void)
update_controlfile(DataDir, ControlFile, true);
}
+/*
+ * Useful to set only ControlFile's database state.
+ */
+static void
+SetControlFileDBState(DBState state)
+{
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+ ControlFile->state = state;
+ UpdateControlFile();
+ LWLockRelease(ControlFileLock);
+}
+
/*
* Returns the unique system identifier from control file.
*/
@@ -9137,12 +9149,7 @@ CreateCheckPoint(int flags)
START_CRIT_SECTION();
if (shutdown)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNING;
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNING);
/*
* Let smgr prepare for checkpoint; this has to happen before we determine
@@ -9732,12 +9739,8 @@ CreateRestartPoint(int flags)
UpdateMinRecoveryPoint(InvalidXLogRecPtr, true);
if (flags & CHECKPOINT_IS_SHUTDOWN)
- {
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
- LWLockRelease(ControlFileLock);
- }
+ SetControlFileDBState(DB_SHUTDOWNED_IN_RECOVERY);
+
return false;
}
--
2.18.0
On Thu, Nov 25, 2021 at 10:21:40AM +0530, Amul Sul wrote:
Thanks for the inputs -- moved timestamp setting inside update_controlfile().
I have not check the performance implication of that with a micro
benchmark or the like, but I can get behind 0001 on consistency
grounds between the backend and the frontend. 0002 does not seem
worth the trouble, though, as it is changing only two code paths.
--
Michael
On Thu, Nov 25, 2021 at 04:04:23PM +0900, Michael Paquier wrote:
I have not check the performance implication of that with a micro
benchmark or the like, but I can get behind 0001 on consistency
grounds between the backend and the frontend.
/* Now create pg_control */
InitControlFile(sysidentifier);
- ControlFile->time = checkPoint.time;
ControlFile->checkPoint = checkPoint.redo;
ControlFile->checkPointCopy = checkPoint;
0001 has a mistake here, no? The initial control file creation goes
through WriteControlFile(), and not update_controlfile(), so this
change means that we would miss setting up this timestamp for the
first time.
@@ -714,7 +714,6 @@ GuessControlValues(void)
ControlFile.checkPointCopy.oldestActiveXid = InvalidTransactionId;
ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
This one had better not be removed, either, as we require pg_resetwal
to guess a set of control file values. Removing the one in
RewriteControlFile() is fine, on the contrary.
--
Michael
On Fri, Nov 26, 2021 at 12:16 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 25, 2021 at 04:04:23PM +0900, Michael Paquier wrote:
I have not check the performance implication of that with a micro
benchmark or the like, but I can get behind 0001 on consistency
grounds between the backend and the frontend./* Now create pg_control */
InitControlFile(sysidentifier);
- ControlFile->time = checkPoint.time;
ControlFile->checkPoint = checkPoint.redo;
ControlFile->checkPointCopy = checkPoint;
0001 has a mistake here, no? The initial control file creation goes
through WriteControlFile(), and not update_controlfile(), so this
change means that we would miss setting up this timestamp for the
first time.@@ -714,7 +714,6 @@ GuessControlValues(void)
ControlFile.checkPointCopy.oldestActiveXid = InvalidTransactionId;ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
This one had better not be removed, either, as we require pg_resetwal
to guess a set of control file values. Removing the one in
RewriteControlFile() is fine, on the contrary.
My bad, sorry for the sloppy change, corrected it in the attached version.
Regards,
Amul
Attachments:
v6-0001-Do-the-ControlFile-timestamp-setting-in-update_co.patchapplication/x-patch; name=v6-0001-Do-the-ControlFile-timestamp-setting-in-update_co.patchDownload
From a2c385f6a6152dbba1e33149f1d7f102243ed0cd Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 23 Sep 2021 00:47:52 -0400
Subject: [PATCH v6] Do the ControlFile timestamp setting in
update_controlfile.
---
src/backend/access/transam/xlog.c | 8 +-------
src/bin/pg_resetwal/pg_resetwal.c | 1 -
src/common/controldata_utils.c | 4 ++++
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b54ec549705..b980c6ac21c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7339,7 +7339,7 @@ StartupXLOG(void)
ControlFile->backupEndPoint = ControlFile->minRecoveryPoint;
}
}
- ControlFile->time = (pg_time_t) time(NULL);
+
/* No need to hold ControlFileLock yet, we aren't up far enough */
UpdateControlFile();
@@ -8199,7 +8199,6 @@ StartupXLOG(void)
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
@@ -9142,7 +9141,6 @@ CreateCheckPoint(int flags)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNING;
- ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
LWLockRelease(ControlFileLock);
}
@@ -9412,7 +9410,6 @@ CreateCheckPoint(int flags)
ControlFile->state = DB_SHUTDOWNED;
ControlFile->checkPoint = ProcLastRecPtr;
ControlFile->checkPointCopy = checkPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/* crash recovery should always recover to the end of WAL */
ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
ControlFile->minRecoveryPointTLI = 0;
@@ -9539,7 +9536,6 @@ CreateEndOfRecoveryRecord(void)
* changes to this point.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->time = (pg_time_t) time(NULL);
ControlFile->minRecoveryPoint = recptr;
ControlFile->minRecoveryPointTLI = xlrec.ThisTimeLineID;
UpdateControlFile();
@@ -9740,7 +9736,6 @@ CreateRestartPoint(int flags)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
LWLockRelease(ControlFileLock);
}
@@ -9801,7 +9796,6 @@ CreateRestartPoint(int flags)
{
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
/*
* Ensure minRecoveryPoint is past the checkpoint record. Normally,
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 84b6f70ad6a..c0ab392c3a2 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -911,7 +911,6 @@ RewriteControlFile(void)
ControlFile.checkPointCopy.time = (pg_time_t) time(NULL);
ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
ControlFile.checkPoint = ControlFile.checkPointCopy.redo;
ControlFile.minRecoveryPoint = 0;
ControlFile.minRecoveryPointTLI = 0;
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 7d4af7881ea..4ce097c05ce 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -23,6 +23,7 @@
#include <unistd.h>
#include <sys/stat.h>
#include <fcntl.h>
+#include <time.h>
#include "access/xlog_internal.h"
#include "catalog/pg_control.h"
@@ -168,6 +169,9 @@ update_controlfile(const char *DataDir,
StaticAssertStmt(sizeof(ControlFileData) <= PG_CONTROL_FILE_SIZE,
"sizeof(ControlFileData) exceeds PG_CONTROL_FILE_SIZE");
+ /* Update timestamp */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/* Recalculate CRC of control file */
INIT_CRC32C(ControlFile->crc);
COMP_CRC32C(ControlFile->crc,
--
2.18.0
On Fri, Nov 26, 2021 at 2:48 PM Amul Sul <sulamul@gmail.com> wrote:
ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
This one had better not be removed, either, as we require pg_resetwal
to guess a set of control file values. Removing the one in
RewriteControlFile() is fine, on the contrary.My bad, sorry for the sloppy change, corrected it in the attached version.
Thanks for the patch. By moving the time update to update_controlfile,
the patch ensures that we have the correct last updated time. Earlier
we were missing (in some places) to update the time before calling
UpdateControlFile.
Isn't it better if we update the ControlFile->time at the end of the
update_controlfile, after file write/sync?
Why do we even need UpdateControlFile which just calls another
function? It may be there for usability and readability, but can't the
pg backend code just call update_controlfile(DataDir, ControlFile,
true); directly so that a function call cost can be avoided?
Otherwise, why can't we make UpdateControlFile an inline function? I'm
not sure if any of the compilers will ever optimize by inlining it
without the "inline" keyword.
Regards,
Bharath Rupireddy.
On Sun, Nov 28, 2021 at 07:53:13AM +0530, Bharath Rupireddy wrote:
Isn't it better if we update the ControlFile->time at the end of the
update_controlfile, after file write/sync?
I don't quite understand your point here. We want to update the
control file's timestamp when it is written, before calculating its
CRC.
Why do we even need UpdateControlFile which just calls another
function? It may be there for usability and readability, but can't the
pg backend code just call update_controlfile(DataDir, ControlFile,
true); directly so that a function call cost can be avoided?
Otherwise, why can't we make UpdateControlFile an inline function? I'm
not sure if any of the compilers will ever optimize by inlining it
without the "inline" keyword.
I would leave it as-is as UpdateControlFile() is a public API old
enough to vote (a70e74b0). Anyway, that's a useful wrapper for the
backend.
--
Michael
On Sun, Nov 28, 2021 at 10:03 AM Michael Paquier <michael@paquier.xyz> wrote:
We want to update the
control file's timestamp when it is written, before calculating its
CRC.
Okay.
Why do we even need UpdateControlFile which just calls another
function? It may be there for usability and readability, but can't the
pg backend code just call update_controlfile(DataDir, ControlFile,
true); directly so that a function call cost can be avoided?
Otherwise, why can't we make UpdateControlFile an inline function? I'm
not sure if any of the compilers will ever optimize by inlining it
without the "inline" keyword.I would leave it as-is as UpdateControlFile() is a public API old
enough to vote (a70e74b0). Anyway, that's a useful wrapper for the
backend.
In that case, why can't we inline UpdateControlFile to avoid the
function call cost? Do you see any issues with it?
BTW, the v6 patch proposed by Amul at [1]/messages/by-id/CAAJ_b94_s-VQs3Vtn_X-ReYr1DzaEakwPi80D1UYSmV3-f+_pw@mail.gmail.com, looks good to me.
[1]: /messages/by-id/CAAJ_b94_s-VQs3Vtn_X-ReYr1DzaEakwPi80D1UYSmV3-f+_pw@mail.gmail.com
Regards,
Bharath Rupireddy.
On Mon, Nov 29, 2021 at 09:28:23AM +0530, Bharath Rupireddy wrote:
In that case, why can't we inline UpdateControlFile to avoid the
function call cost? Do you see any issues with it?
This routine is IMO not something worth bothering about.
BTW, the v6 patch proposed by Amul at [1], looks good to me.
Yes, I have no problems with this part, so done.
--
Michael
On Mon, Nov 29, 2021 at 10:12 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Nov 29, 2021 at 09:28:23AM +0530, Bharath Rupireddy wrote:
In that case, why can't we inline UpdateControlFile to avoid the
function call cost? Do you see any issues with it?This routine is IMO not something worth bothering about.
BTW, the v6 patch proposed by Amul at [1], looks good to me.
Yes, I have no problems with this part, so done.
Thank you, Michael.
Regards,
Amul