On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossa...@amazon.com> wrote:
>
> On 9/19/21, 11:07 PM, "Amul Sul" <sula...@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
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
