Thanks for merging. It still applies on the current master with
some displacements.
At Wed, 5 Oct 2016 15:18:53 +0900, Michael Paquier <michael.paqu...@gmail.com>
wrote in <CAB7nPqT4U=osolxufuxmonmfdqfmd5f_0dmkoddvjg-hhwq...@mail.gmail.com>
> (Squashing replies)
>
> On Fri, Sep 30, 2016 at 6:13 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
> > On Fri, Sep 30, 2016 at 2:51 PM, Michael Paquier
> > <michael.paqu...@gmail.com> wrote:
> >> On Fri, Sep 30, 2016 at 2:05 PM, Kyotaro HORIGUCHI
> >> <horiguchi.kyot...@lab.ntt.co.jp> wrote:
> >>> At Fri, 30 Sep 2016 14:00:15 +0900 (Tokyo Standard Time), Kyotaro
> >>> HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote in
> >>> <20160930.140015.150178454.horiguchi.kyot...@lab.ntt.co.jp>
> >>>> I don't see no problem in setting progressAt in XLogInsertRecord.
> >>>> But I doubt GetProgressRecPtr is harmful, especially when
> >>>
> >>> But I suspect that GetProgressRecPtr could be harmful.
> >>
> >> Well, you can maximize its effects by doing NUM_XLOGINSERT_LOCKS ==
> >> nproc and reducing checkpoint_timeout. That's what I did but..
> >
> > Note: I am moving this patch to next CF.
>
> And I am back on it more seriously... And I am taking back what I said
> upthread.
>
> I looked at the v12 that Horiguchi-san has written, and that seems
> correct to me. So I have squashed everything into a single patch,
Could you let me struggle a bit more to avoid LWLocks in
GetProgressRecPtr?
I considered two alternatives for updating logic of progressAt
more seriously. One is, as Amit suggested, replacing progressAt
within the SpinLock section in
ReserverXLogInsertLocation. Another is using pg_atomic_u64 for
progressAt. The attached two patches rouhgly implement the aboves
respectively. (But I've not tested them. The patches are to show
the two alternatives concretely.)
I found that the former reuiqres to take insertpos_lck also on
reading. I have to admit that this is too bad. (Even I saw no
degradation by pgbench on my poor environment. It marks 118tr/s
by 10 threads and that doesn't seem make any stress on xlog
logics...)
For the latter, it is free from locks and doesn't reduce parallel
degree but I'm not sure it is proper to use it there and I'm not
sure about actual overheads. In the worst case, it makes another
SpinLock section for every call to pg_atmoic_* functions.
> including the log entry that gets logged with log_checkpoints. Testing
> with archive_timeout to 10s, checkpoint_timeout to 30s, sometimes
> triggering manual activity with CREATE TABLE/whatever and manual
> pg_switch_xlog(), I am able to see that checkpoints can be correctly
> skipped or generated.
>
> There was as well a compilation error with ereport(). Not sure how I
> missed that... Likely too many patches handled these days.
>
> I have also updated the description of archive_timeout that increasing
> checkpoint_timeout would reduce unnecessary checkpoints on a idle
> system. With this patch, on an idle system checkpoints are just
> skipped as they should.
>
> How does that look?
All except the above point looks good for me. Maybe it is better
that XLOG_INCLUDE_ORIGIN stuff is in a separate patch.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From e09147a91d20e3c86da26e311518f92aea193bc1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Tue, 1 Nov 2016 18:28:58 +0900
Subject: [PATCH] Alternative Type 1.
Maintenance progressAt in SpinLock(Insert->insertpos_lck) section in
ReserveXLogInsertLocation. Even if additinal code into the section
does no harm, GetProgressRecPtr also needs to take the same lock.
---
src/backend/access/transam/xlog.c | 56 ++++++++++-----------------------------
1 file changed, 14 insertions(+), 42 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1eff059..0703e5b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -466,7 +466,6 @@ typedef struct
{
LWLock lock;
XLogRecPtr insertingAt;
- XLogRecPtr progressAt;
} WALInsertLock;
/*
@@ -672,9 +671,13 @@ typedef struct XLogCtlData
*/
XLogRecPtr lastFpwDisableRecPtr;
+ uint64 progressAt; /* This is not locked by info_lck but it is
+ * here for just convenience. */
+
slock_t info_lck; /* locks shared variables shown above */
} XLogCtlData;
+
static XLogCtlData *XLogCtl = NULL;
/* a private copy of XLogCtl->Insert.WALInsertLocks, for convenience */
@@ -1021,25 +1024,6 @@ XLogInsertRecord(XLogRecData *rdata,
inserted = true;
}
- /*
- * Update the progress LSN positions. At least one WAL insertion lock
- * is already taken appropriately before doing that, and it is just more
- * simple to do that here where WAL record data and type is at hand.
- * The progress is set at the start position of the record tracked that
- * is being added, making easier checkpoint progress tracking as the
- * control file already saves the start LSN position of the last
- * checkpoint run. If an exclusive lock is taken for WAL insertion,
- * there is actually no need to update all the progression fields, so
- * just do it on the first one.
- */
- if ((flags & XLOG_NO_PROGRESS) == 0)
- {
- if (holdingAllLocks)
- WALInsertLocks[0].l.progressAt = StartPos;
- else
- WALInsertLocks[MyLockNo].l.progressAt = StartPos;
- }
-
if (inserted)
{
/*
@@ -1195,6 +1179,7 @@ static void
ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
XLogRecPtr *PrevPtr)
{
+ volatile XLogCtlData *xlogctl = XLogCtl;
XLogCtlInsert *Insert = &XLogCtl->Insert;
uint64 startbytepos;
uint64 endbytepos;
@@ -1222,6 +1207,8 @@ ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
prevbytepos = Insert->PrevBytePos;
Insert->CurrBytePos = endbytepos;
Insert->PrevBytePos = startbytepos;
+ if (xlogctl->progressAt < startbytepos)
+ xlogctl->progressAt = startbytepos;
SpinLockRelease(&Insert->insertpos_lck);
@@ -4763,7 +4750,6 @@ XLOGShmemInit(void)
{
LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT);
WALInsertLocks[i].l.insertingAt = InvalidXLogRecPtr;
- WALInsertLocks[i].l.progressAt = InvalidXLogRecPtr;
}
/*
@@ -8050,29 +8036,15 @@ GetFlushRecPtr(void)
XLogRecPtr
GetProgressRecPtr(void)
{
- XLogRecPtr res = InvalidXLogRecPtr;
- int i;
+ volatile XLogCtlData *xlogctl = XLogCtl;
+ uint32 bret;
- /*
- * Look at the latest LSN position referring to the activity done by
- * WAL insertion. An exclusive lock is taken because currently the
- * locking logic for WAL insertion only expects such a level of locking.
- * Taking a lock is as well necessary to prevent potential torn reads
- * on some platforms.
- */
- for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
- {
- XLogRecPtr progress_lsn;
-
- LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
- progress_lsn = WALInsertLocks[i].l.progressAt;
- LWLockRelease(&WALInsertLocks[i].l.lock);
-
- if (res < progress_lsn)
- res = progress_lsn;
- }
+ /* XXXX: Taking insertpos_lck is too bad but it's necessary :( */
+ SpinLockAcquire(&xlogctl->Insert.insertpos_lck);
+ bret = xlogctl->progressAt;
+ SpinLockRelease(&xlogctl->Insert.insertpos_lck);
- return res;
+ return XLogBytePosToRecPtr(bret);
}
/*
--
2.9.2
>From cc0167a03c35a5630983b5bed725913383b47ed4 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Tue, 1 Nov 2016 18:28:58 +0900
Subject: [PATCH] Alternative Type 2 atomic.
Use atomic_u64 for holding progressAt.
---
src/backend/access/transam/xlog.c | 42 +++++++++++----------------------------
1 file changed, 12 insertions(+), 30 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1eff059..bc78e9e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -466,7 +466,6 @@ typedef struct
{
LWLock lock;
XLogRecPtr insertingAt;
- XLogRecPtr progressAt;
} WALInsertLock;
/*
@@ -672,9 +671,13 @@ typedef struct XLogCtlData
*/
XLogRecPtr lastFpwDisableRecPtr;
+ pg_atomic_uint64 progressAt; /* This is not locked by info_lck but this
+ * is here for just convenient now */
+
slock_t info_lck; /* locks shared variables shown above */
} XLogCtlData;
+
static XLogCtlData *XLogCtl = NULL;
/* a private copy of XLogCtl->Insert.WALInsertLocks, for convenience */
@@ -1021,23 +1024,13 @@ XLogInsertRecord(XLogRecData *rdata,
inserted = true;
}
- /*
- * Update the progress LSN positions. At least one WAL insertion lock
- * is already taken appropriately before doing that, and it is just more
- * simple to do that here where WAL record data and type is at hand.
- * The progress is set at the start position of the record tracked that
- * is being added, making easier checkpoint progress tracking as the
- * control file already saves the start LSN position of the last
- * checkpoint run. If an exclusive lock is taken for WAL insertion,
- * there is actually no need to update all the progression fields, so
- * just do it on the first one.
- */
- if ((flags & XLOG_NO_PROGRESS) == 0)
{
- if (holdingAllLocks)
- WALInsertLocks[0].l.progressAt = StartPos;
- else
- WALInsertLocks[MyLockNo].l.progressAt = StartPos;
+ volatile XLogCtlData *xlogctl = XLogCtl;
+ XLogRecPtr tmpstartpos = pg_atomic_read_u64(&xlogctl->progressAt);
+
+ while (tmpstartpos < StartPos)
+ pg_atomic_compare_exchange_u64(&xlogctl->progressAt,
+ &tmpstartpos, StartPos);
}
if (inserted)
@@ -4763,7 +4756,6 @@ XLOGShmemInit(void)
{
LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT);
WALInsertLocks[i].l.insertingAt = InvalidXLogRecPtr;
- WALInsertLocks[i].l.progressAt = InvalidXLogRecPtr;
}
/*
@@ -8051,7 +8043,7 @@ XLogRecPtr
GetProgressRecPtr(void)
{
XLogRecPtr res = InvalidXLogRecPtr;
- int i;
+ volatile XLogCtlData *xlogctl = XLogCtl;
/*
* Look at the latest LSN position referring to the activity done by
@@ -8060,17 +8052,7 @@ GetProgressRecPtr(void)
* Taking a lock is as well necessary to prevent potential torn reads
* on some platforms.
*/
- for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++)
- {
- XLogRecPtr progress_lsn;
-
- LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
- progress_lsn = WALInsertLocks[i].l.progressAt;
- LWLockRelease(&WALInsertLocks[i].l.lock);
-
- if (res < progress_lsn)
- res = progress_lsn;
- }
+ res = pg_atomic_read_u64(&xlogctl->progressAt);
return res;
}
--
2.9.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
