public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1] Hide other user's pg_stat_ssl rows
20+ messages / 6 participants
[nested] [flat]
* [PATCH v1] Hide other user's pg_stat_ssl rows
@ 2019-02-20 10:38 Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Peter Eisentraut @ 2019-02-20 10:38 UTC (permalink / raw)
Change pg_stat_ssl so that an unprivileged user can only see their own
rows; other rows will be all null. This makes the behavior consistent
with pg_stat_activity, where information about where the connection
came from is also restricted.
---
src/backend/utils/adt/pgstatfuncs.c | 73 ++++++++++++++++-------------
1 file changed, 41 insertions(+), 32 deletions(-)
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index b6ba856ebe..69f7265779 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -645,38 +645,6 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
else
nulls[16] = true;
- if (beentry->st_ssl)
- {
- values[18] = BoolGetDatum(true); /* ssl */
- values[19] = CStringGetTextDatum(beentry->st_sslstatus->ssl_version);
- values[20] = CStringGetTextDatum(beentry->st_sslstatus->ssl_cipher);
- values[21] = Int32GetDatum(beentry->st_sslstatus->ssl_bits);
- values[22] = BoolGetDatum(beentry->st_sslstatus->ssl_compression);
-
- if (beentry->st_sslstatus->ssl_client_dn[0])
- values[23] = CStringGetTextDatum(beentry->st_sslstatus->ssl_client_dn);
- else
- nulls[23] = true;
-
- if (beentry->st_sslstatus->ssl_client_serial[0])
- values[24] = DirectFunctionCall3(numeric_in,
- CStringGetDatum(beentry->st_sslstatus->ssl_client_serial),
- ObjectIdGetDatum(InvalidOid),
- Int32GetDatum(-1));
- else
- nulls[24] = true;
-
- if (beentry->st_sslstatus->ssl_issuer_dn[0])
- values[25] = CStringGetTextDatum(beentry->st_sslstatus->ssl_issuer_dn);
- else
- nulls[25] = true;
- }
- else
- {
- values[18] = BoolGetDatum(false); /* ssl */
- nulls[19] = nulls[20] = nulls[21] = nulls[22] = nulls[23] = nulls[24] = nulls[25] = true;
- }
-
/* Values only available to role member or pg_read_all_stats */
if (has_privs_of_role(GetUserId(), beentry->st_userid) ||
is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS))
@@ -854,6 +822,39 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
else
values[17] =
CStringGetTextDatum(pgstat_get_backend_desc(beentry->st_backendType));
+
+ /* SSL information */
+ if (beentry->st_ssl)
+ {
+ values[18] = BoolGetDatum(true); /* ssl */
+ values[19] = CStringGetTextDatum(beentry->st_sslstatus->ssl_version);
+ values[20] = CStringGetTextDatum(beentry->st_sslstatus->ssl_cipher);
+ values[21] = Int32GetDatum(beentry->st_sslstatus->ssl_bits);
+ values[22] = BoolGetDatum(beentry->st_sslstatus->ssl_compression);
+
+ if (beentry->st_sslstatus->ssl_client_dn[0])
+ values[23] = CStringGetTextDatum(beentry->st_sslstatus->ssl_client_dn);
+ else
+ nulls[23] = true;
+
+ if (beentry->st_sslstatus->ssl_client_serial[0])
+ values[24] = DirectFunctionCall3(numeric_in,
+ CStringGetDatum(beentry->st_sslstatus->ssl_client_serial),
+ ObjectIdGetDatum(InvalidOid),
+ Int32GetDatum(-1));
+ else
+ nulls[24] = true;
+
+ if (beentry->st_sslstatus->ssl_issuer_dn[0])
+ values[25] = CStringGetTextDatum(beentry->st_sslstatus->ssl_issuer_dn);
+ else
+ nulls[25] = true;
+ }
+ else
+ {
+ values[18] = BoolGetDatum(false); /* ssl */
+ nulls[19] = nulls[20] = nulls[21] = nulls[22] = nulls[23] = nulls[24] = nulls[25] = true;
+ }
}
else
{
@@ -870,6 +871,14 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[13] = true;
nulls[14] = true;
nulls[17] = true;
+ nulls[18] = true;
+ nulls[19] = true;
+ nulls[20] = true;
+ nulls[21] = true;
+ nulls[22] = true;
+ nulls[23] = true;
+ nulls[24] = true;
+ nulls[25] = true;
}
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
base-commit: 56fadbedbd2f697400b89e7b767cfa4ec67932d6
--
2.20.1
--------------C4872B86007236D68CC45575--
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
@ 2022-02-17 05:16 Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-17 05:16 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]
At Wed, 16 Feb 2022 10:40:56 +0300, Yura Sokolov <[email protected]> wrote in
> Hello, all.
>
> I thought about patch simplification, and tested version
> without BufTable and dynahash api change at all.
>
> It performs suprisingly well. It is just a bit worse
> than v1 since there is more contention around dynahash's
> freelist, but most of improvement remains.
>
> I'll finish benchmarking and will attach graphs with
> next message. Patch is attached here.
Thanks for the new patch. The patch as a whole looks fine to me. But
some comments needs to be revised.
(existing comments)
> * To change the association of a valid buffer, we'll need to have
> * exclusive lock on both the old and new mapping partitions.
...
> * Somebody could have pinned or re-dirtied the buffer while we were
> * doing the I/O and making the new hashtable entry. If so, we can't
> * recycle this buffer; we must undo everything we've done and start
> * over with a new victim buffer.
We no longer take a lock on the new partition and have no new hash
entry (if others have not yet done) at this point.
+ * Clear out the buffer's tag and flags. We must do this to ensure that
+ * linear scans of the buffer array don't think the buffer is valid. We
The reason we can clear out the tag is it's safe to use the victim
buffer at this point. This comment needs to mention that reason.
+ *
+ * Since we are single pinner, there should no be PIN_COUNT_WAITER or
+ * IO_IN_PROGRESS (flags that were not cleared in previous code).
+ */
+ Assert((oldFlags & (BM_PIN_COUNT_WAITER | BM_IO_IN_PROGRESS)) == 0);
It seems like to be a test for potential bugs in other functions. As
the comment is saying, we are sure that no other processes are pinning
the buffer and the existing code doesn't seem to be care about that
condition. Is it really needed?
+ /*
+ * Try to make a hashtable entry for the buffer under its new tag. This
+ * could fail because while we were writing someone else allocated another
The most significant point of this patch is the reason that the victim
buffer is protected from stealing until it is set up for new tag. I
think we need an explanation about the protection here.
+ * buffer for the same block we want to read in. Note that we have not yet
+ * removed the hashtable entry for the old tag.
Since we have removed the hash table entry for the old tag at this
point, the comment got wrong.
+ * the first place. First, give up the buffer we were planning to use
+ * and put it to free lists.
..
+ StrategyFreeBuffer(buf);
This is one downside of this patch. But it seems to me that the odds
are low that many buffers are freed in a short time by this logic. By
the way it would be better if the sentence starts with "First" has a
separate comment section.
(existing comment)
| * Okay, it's finally safe to rename the buffer.
We don't "rename" the buffer here. And the safety is already
establishsed at the end of the oldPartitionLock section. So it would
be just something like "Now allocate the victim buffer for the new
tag"?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
@ 2022-02-21 08:06 ` Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 08:04 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Yura Sokolov @ 2022-02-21 08:06 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]
Good day, Kyotaro Horiguchi and hackers.
В Чт, 17/02/2022 в 14:16 +0900, Kyotaro Horiguchi пишет:
> At Wed, 16 Feb 2022 10:40:56 +0300, Yura Sokolov <[email protected]> wrote in
> > Hello, all.
> >
> > I thought about patch simplification, and tested version
> > without BufTable and dynahash api change at all.
> >
> > It performs suprisingly well. It is just a bit worse
> > than v1 since there is more contention around dynahash's
> > freelist, but most of improvement remains.
> >
> > I'll finish benchmarking and will attach graphs with
> > next message. Patch is attached here.
>
> Thanks for the new patch. The patch as a whole looks fine to me. But
> some comments needs to be revised.
Thank you for review and remarks.
>
> (existing comments)
> > * To change the association of a valid buffer, we'll need to have
> > * exclusive lock on both the old and new mapping partitions.
> ...
> > * Somebody could have pinned or re-dirtied the buffer while we were
> > * doing the I/O and making the new hashtable entry. If so, we can't
> > * recycle this buffer; we must undo everything we've done and start
> > * over with a new victim buffer.
>
> We no longer take a lock on the new partition and have no new hash
> entry (if others have not yet done) at this point.
fixed
> + * Clear out the buffer's tag and flags. We must do this to ensure that
> + * linear scans of the buffer array don't think the buffer is valid. We
>
> The reason we can clear out the tag is it's safe to use the victim
> buffer at this point. This comment needs to mention that reason.
Tried to describe.
> + *
> + * Since we are single pinner, there should no be PIN_COUNT_WAITER or
> + * IO_IN_PROGRESS (flags that were not cleared in previous code).
> + */
> + Assert((oldFlags & (BM_PIN_COUNT_WAITER | BM_IO_IN_PROGRESS)) == 0);
>
> It seems like to be a test for potential bugs in other functions. As
> the comment is saying, we are sure that no other processes are pinning
> the buffer and the existing code doesn't seem to be care about that
> condition. Is it really needed?
Ok, I agree this check is excess.
These two flags were not cleared in the previous code, and I didn't get
why. Probably, it is just a historical accident.
>
> + /*
> + * Try to make a hashtable entry for the buffer under its new tag. This
> + * could fail because while we were writing someone else allocated another
>
> The most significant point of this patch is the reason that the victim
> buffer is protected from stealing until it is set up for new tag. I
> think we need an explanation about the protection here.
I don't get what you mean clearly :( . I would appreciate your
suggestion for this comment.
>
>
> + * buffer for the same block we want to read in. Note that we have not yet
> + * removed the hashtable entry for the old tag.
>
> Since we have removed the hash table entry for the old tag at this
> point, the comment got wrong.
Thanks, changed.
> + * the first place. First, give up the buffer we were planning to use
> + * and put it to free lists.
> ..
> + StrategyFreeBuffer(buf);
>
> This is one downside of this patch. But it seems to me that the odds
> are low that many buffers are freed in a short time by this logic. By
> the way it would be better if the sentence starts with "First" has a
> separate comment section.
Splitted the comment.
> (existing comment)
> | * Okay, it's finally safe to rename the buffer.
>
> We don't "rename" the buffer here. And the safety is already
> establishsed at the end of the oldPartitionLock section. So it would
> be just something like "Now allocate the victim buffer for the new
> tag"?
Changed to "Now it is safe to use victim buffer for new tag."
There is also tiny code change at block reuse finalization: instead
of LockBufHdr+UnlockBufHdr I use single atomic_fetch_or protected
with WaitBufHdrUnlocked. I've tried to explain its safety. Please,
check it.
Benchmarks:
- base point is 6ce16088bfed97f9.
- notebook with i7-1165G7 and server with Xeon 8354H (1&2 sockets)
- pgbench select only scale 100 (1.5GB on disk)
- two shared_buffers values: 128MB and 1GB.
- enabled hugepages
- second best result from five runs
Notebook:
conns | master | patch_v3 | master 1G | patch_v3 1G
--------+------------+------------+------------+------------
1 | 29508 | 29481 | 31774 | 32305
2 | 57139 | 56694 | 63393 | 62968
3 | 89759 | 90861 | 101873 | 102399
5 | 133491 | 134573 | 145451 | 145009
7 | 156739 | 155832 | 164633 | 164562
17 | 216863 | 216379 | 251923 | 251017
27 | 209532 | 209802 | 244202 | 243709
53 | 212615 | 213552 | 248107 | 250317
83 | 214446 | 218230 | 252414 | 252337
107 | 211276 | 217109 | 252762 | 250328
139 | 208070 | 214265 | 248350 | 249684
163 | 206764 | 214594 | 247369 | 250323
191 | 205478 | 213511 | 244597 | 246877
211 | 200976 | 212976 | 244035 | 245032
239 | 196588 | 211519 | 243897 | 245055
271 | 195813 | 209631 | 237457 | 242771
307 | 192724 | 208074 | 237658 | 241759
353 | 187847 | 207189 | 234548 | 239008
397 | 186942 | 205317 | 230465 | 238782
I don't get why numbers changed from first letter ))
But still no slowdown, and measurable gain at 128MB shared
buffers.
Xeon 1 socket
conns | master | patch_v3 | master 1G | patch_v3 1G
--------+------------+------------+------------+------------
1 | 41975 | 41799 | 52898 | 52715
2 | 77693 | 77531 | 97571 | 98547
3 | 114713 | 114533 | 142709 | 143579
5 | 188898 | 187241 | 239322 | 236682
7 | 261516 | 260249 | 329119 | 328562
17 | 521821 | 518981 | 672390 | 662987
27 | 555487 | 557019 | 674630 | 675703
53 | 868213 | 897097 | 1190734 | 1202575
83 | 868232 | 881705 | 1164997 | 1157764
107 | 850477 | 855169 | 1140597 | 1128027
139 | 816311 | 826756 | 1101471 | 1096197
163 | 794788 | 805946 | 1078445 | 1071535
191 | 765934 | 783209 | 1059497 | 1039936
211 | 738656 | 786171 | 1083356 | 1049025
239 | 713124 | 837040 | 1104629 | 1125969
271 | 692138 | 847741 | 1094432 | 1131968
307 | 682919 | 847939 | 1086306 | 1124649
353 | 679449 | 844596 | 1071482 | 1125980
397 | 676217 | 833009 | 1058937 | 1113496
Here is small slowdown at some connection numbers (17,
107-191).It is reproducible. Probably it is due to one more
atomice write. Perhaps for some other scheduling issues (
processes block less on buffer manager but compete more
on other resources). I could not reliably determine why,
because change is too small, and `perf record` harms
performance more at this point.
This is the reason I've changed finalization to atomic_or
instead of Lock+Unlock pair. The changed helped a bit, but
didn't remove slowdown completely.
Xeon 2 socket
conns | m0 | patch_v3 | m0 1G | patch_v3 1G
--------+------------+------------+------------+------------
1 | 44317 | 43747 | 53920 | 53759
2 | 81193 | 79976 | 99138 | 99213
3 | 120755 | 114481 | 148102 | 146494
5 | 190007 | 187384 | 232078 | 229627
7 | 258602 | 256657 | 325545 | 322417
17 | 551814 | 549041 | 692312 | 688204
27 | 787353 | 787916 | 1023509 | 1020995
53 | 973880 | 996019 | 1228274 | 1246128
83 | 1108442 | 1258589 | 1596292 | 1662586
107 | 1072188 | 1317229 | 1542401 | 1684603
139 | 1000446 | 1272759 | 1490757 | 1672507
163 | 967378 | 1224513 | 1461468 | 1660012
191 | 926010 | 1178067 | 1435317 | 1645886
211 | 909919 | 1148862 | 1417437 | 1629487
239 | 895944 | 1108579 | 1393530 | 1616824
271 | 880545 | 1078280 | 1374878 | 1608412
307 | 865560 | 1056988 | 1355164 | 1601066
353 | 857591 | 1033980 | 1330069 | 1586769
397 | 840374 | 1016690 | 1312257 | 1573376
regards,
Yura Sokolov
Postgres Professional
[email protected]
[email protected]
Attachments:
[text/x-patch] v3-0001-PGPRO-5616-bufmgr-do-not-acquire-two-partition-lo.patch (9.5K, ../../[email protected]/2-v3-0001-PGPRO-5616-bufmgr-do-not-acquire-two-partition-lo.patch)
download | inline diff:
From 04b07d0627ec65ba3327dc8338d59dbd15c405d8 Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Mon, 21 Feb 2022 08:49:03 +0300
Subject: [PATCH v3] [PGPRO-5616] bufmgr: do not acquire two partition locks.
Acquiring two partition locks leads to complex dependency chain that hurts
at high concurrency level.
There is no need to hold both lock simultaneously. Buffer is pinned so
other processes could not select it for eviction. If tag is cleared and
buffer removed from old partition other processes will not find it.
Therefore it is safe to release old partition lock before acquiring
new partition lock.
Tags: lwlock_numa
---
src/backend/storage/buffer/bufmgr.c | 206 ++++++++++++++--------------
1 file changed, 105 insertions(+), 101 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b4532948d3f..bb8b1cd2f4b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1114,6 +1114,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
BufferDesc *buf;
bool valid;
uint32 buf_state;
+ uint32 new_bits;
/* create a tag so we can lookup the buffer */
INIT_BUFFERTAG(newTag, smgr->smgr_rnode.node, forkNum, blockNum);
@@ -1288,93 +1289,16 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
oldHash = BufTableHashCode(&oldTag);
oldPartitionLock = BufMappingPartitionLock(oldHash);
- /*
- * Must lock the lower-numbered partition first to avoid
- * deadlocks.
- */
- if (oldPartitionLock < newPartitionLock)
- {
- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- }
- else if (oldPartitionLock > newPartitionLock)
- {
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
- }
- else
- {
- /* only one partition, only one lock */
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- }
+ LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
}
else
{
- /* if it wasn't valid, we need only the new partition */
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
/* remember we have no old-partition lock or tag */
oldPartitionLock = NULL;
/* keep the compiler quiet about uninitialized variables */
oldHash = 0;
}
- /*
- * Try to make a hashtable entry for the buffer under its new tag.
- * This could fail because while we were writing someone else
- * allocated another buffer for the same block we want to read in.
- * Note that we have not yet removed the hashtable entry for the old
- * tag.
- */
- buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
-
- if (buf_id >= 0)
- {
- /*
- * Got a collision. Someone has already done what we were about to
- * do. We'll just handle this as if it were found in the buffer
- * pool in the first place. First, give up the buffer we were
- * planning to use.
- */
- UnpinBuffer(buf, true);
-
- /* Can give up that buffer's mapping partition lock now */
- if (oldPartitionLock != NULL &&
- oldPartitionLock != newPartitionLock)
- LWLockRelease(oldPartitionLock);
-
- /* remaining code should match code at top of routine */
-
- buf = GetBufferDescriptor(buf_id);
-
- valid = PinBuffer(buf, strategy);
-
- /* Can release the mapping lock as soon as we've pinned it */
- LWLockRelease(newPartitionLock);
-
- *foundPtr = true;
-
- if (!valid)
- {
- /*
- * We can only get here if (a) someone else is still reading
- * in the page, or (b) a previous read attempt failed. We
- * have to wait for any active read attempt to finish, and
- * then set up our own read attempt if the page is still not
- * BM_VALID. StartBufferIO does it all.
- */
- if (StartBufferIO(buf, true))
- {
- /*
- * If we get here, previous attempts to read the buffer
- * must have failed ... but we shall bravely try again.
- */
- *foundPtr = false;
- }
- }
-
- return buf;
- }
-
/*
* Need to lock the buffer header too in order to change its tag.
*/
@@ -1382,52 +1306,132 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/*
* Somebody could have pinned or re-dirtied the buffer while we were
- * doing the I/O and making the new hashtable entry. If so, we can't
- * recycle this buffer; we must undo everything we've done and start
- * over with a new victim buffer.
+ * doing the I/O. If so, we can't recycle this buffer; we must undo
+ * everything we've done and start over with a new victim buffer.
*/
oldFlags = buf_state & BUF_FLAG_MASK;
if (BUF_STATE_GET_REFCOUNT(buf_state) == 1 && !(oldFlags & BM_DIRTY))
break;
UnlockBufHdr(buf, buf_state);
- BufTableDelete(&newTag, newHash);
- if (oldPartitionLock != NULL &&
- oldPartitionLock != newPartitionLock)
+ if (oldPartitionLock != NULL)
LWLockRelease(oldPartitionLock);
- LWLockRelease(newPartitionLock);
UnpinBuffer(buf, true);
}
/*
- * Okay, it's finally safe to rename the buffer.
+ * Clear out the buffer's tag and flags. We must do this to ensure that
+ * linear scans of the buffer array don't think the buffer is valid. We
+ * also reset the usage_count since any recency of use of the old content
+ * is no longer relevant.
*
- * Clearing BM_VALID here is necessary, clearing the dirtybits is just
- * paranoia. We also reset the usage_count since any recency of use of
- * the old content is no longer relevant. (The usage_count starts out at
- * 1 so that the buffer can survive one clock-sweep pass.)
+ * We have pinned buffer and we are single pinner at the moment so there
+ * is no other pinners. We hold buffer header lock and exclusive partition
+ * lock if tag is valid. Given these statements it is safe to clear tag
+ * since no other process can inspect it to the moment.
+ */
+ CLEAR_BUFFERTAG(buf->tag);
+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
+ UnlockBufHdr(buf, buf_state);
+
+ /* Delete old tag from hash table if it were valid. */
+ if (oldFlags & BM_TAG_VALID)
+ BufTableDelete(&oldTag, oldHash);
+
+ if (oldPartitionLock != newPartitionLock)
+ {
+ if (oldPartitionLock != NULL)
+ LWLockRelease(oldPartitionLock);
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ }
+
+ /*
+ * Try to make a hashtable entry for the buffer under its new tag. This
+ * could fail because while we were writing someone else allocated another
+ * buffer for the same block we want to read in. In that case we will have
+ * to return our buffer to free list.
+ */
+ buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
+
+ if (buf_id >= 0)
+ {
+ /*
+ * Got a collision. Someone has already done what we were about to do.
+ * We'll just handle this as if it were found in the buffer pool in
+ * the first place.
+ */
+
+ /*
+ * First, give up the buffer we were planning to use and put it to
+ * free lists.
+ */
+ UnpinBuffer(buf, true);
+ StrategyFreeBuffer(buf);
+
+ /* remaining code should match code at top of routine */
+
+ buf = GetBufferDescriptor(buf_id);
+
+ valid = PinBuffer(buf, strategy);
+
+ /* Can release the mapping lock as soon as we've pinned it */
+ LWLockRelease(newPartitionLock);
+
+ *foundPtr = true;
+
+ if (!valid)
+ {
+ /*
+ * We can only get here if (a) someone else is still reading in
+ * the page, or (b) a previous read attempt failed. We have to
+ * wait for any active read attempt to finish, and then set up our
+ * own read attempt if the page is still not BM_VALID.
+ * StartBufferIO does it all.
+ */
+ if (StartBufferIO(buf, true))
+ {
+ /*
+ * If we get here, previous attempts to read the buffer must
+ * have failed ... but we shall bravely try again.
+ */
+ *foundPtr = false;
+ }
+ }
+
+ return buf;
+ }
+
+ /*
+ * Now it is safe to use victim buffer for new tag.
*
* Make sure BM_PERMANENT is set for buffers that must be written at every
* checkpoint. Unlogged buffers only need to be written at shutdown
* checkpoints, except for their "init" forks, which need to be treated
* just like permanent relations.
+ *
+ * The usage_count starts out at 1 so that the buffer can survive one
+ * clock-sweep pass.
+ *
+ * We use direct atomic OR instead of Lock+Unlock since no other backend
+ * could be interested in the buffer. But StrategyGetBuffer,
+ * Flush*Buffers, Drop*Buffers are scanning all buffers and locks them to
+ * compare tag, and UnlockBufHdr does raw write to state. So we have to
+ * spin if we found buffer locked.
+ *
+ * Note that we write tag unlocked. It is also safe since there is always
+ * check for BM_VALID when tag is compared.
*/
buf->tag = newTag;
- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
- BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
- BUF_USAGECOUNT_MASK);
if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
- buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
+ new_bits = BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
else
- buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
-
- UnlockBufHdr(buf, buf_state);
+ new_bits = BM_TAG_VALID | BUF_USAGECOUNT_ONE;
- if (oldPartitionLock != NULL)
+ buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
+ while (unlikely(buf_state & BM_LOCKED))
{
- BufTableDelete(&oldTag, oldHash);
- if (oldPartitionLock != newPartitionLock)
- LWLockRelease(oldPartitionLock);
+ WaitBufHdrUnlocked(buf);
+ buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
}
LWLockRelease(newPartitionLock);
--
2.35.1
[image/gif] v3-2socket.gif (13.8K, ../../[email protected]/3-v3-2socket.gif)
download | view image
[image/gif] v3-1socket.gif (14.1K, ../../[email protected]/4-v3-1socket.gif)
download | view image
[image/gif] v3-notebook.gif (12.2K, ../../[email protected]/5-v3-notebook.gif)
download | view image
[application/zip] res.zip (46.1K, ../../[email protected]/6-res.zip)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-02-25 04:35 ` Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Simon Riggs @ 2022-02-25 04:35 UTC (permalink / raw)
To: Yura Sokolov <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; Andrey Borodin <[email protected]>; PostgreSQL Developers <[email protected]>
On Mon, 21 Feb 2022 at 08:06, Yura Sokolov <[email protected]> wrote:
>
> Good day, Kyotaro Horiguchi and hackers.
>
> В Чт, 17/02/2022 в 14:16 +0900, Kyotaro Horiguchi пишет:
> > At Wed, 16 Feb 2022 10:40:56 +0300, Yura Sokolov <[email protected]> wrote in
> > > Hello, all.
> > >
> > > I thought about patch simplification, and tested version
> > > without BufTable and dynahash api change at all.
> > >
> > > It performs suprisingly well. It is just a bit worse
> > > than v1 since there is more contention around dynahash's
> > > freelist, but most of improvement remains.
> > >
> > > I'll finish benchmarking and will attach graphs with
> > > next message. Patch is attached here.
> >
> > Thanks for the new patch. The patch as a whole looks fine to me. But
> > some comments needs to be revised.
>
> Thank you for review and remarks.
v3 gets the buffer partition locking right, well done, great results!
In v3, the comment at line 1279 still implies we take both locks
together, which is not now the case.
Dynahash actions are still possible. You now have the BufTableDelete
before the BufTableInsert, which opens up the possibility I discussed
here:
http://postgr.es/m/CANbhV-F0H-8oB_A+m=55hP0e0QRL=RdDDQuSXMTFt6JPrdX+pQ@mail.gmail.com
(Apologies for raising a similar topic, I hadn't noticed this thread
before; thanks to Horiguchi-san for pointing this out).
v1 had a horrible API (sorry!) where you returned the entry and then
explicitly re-used it. I think we *should* make changes to dynahash,
but not with the API you proposed.
Proposal for new BufTable API
BufTableReuse() - similar to BufTableDelete() but does NOT put entry
back on freelist, we remember it in a private single item cache in
dynahash
BufTableAssign() - similar to BufTableInsert() but can only be
executed directly after BufTableReuse(), fails with ERROR otherwise.
Takes the entry from single item cache and re-assigns it to new tag
In dynahash we have two new modes that match the above
HASH_REUSE - used by BufTableReuse(), similar to HASH_REMOVE, but
places entry on the single item cache, avoiding freelist
HASH_ASSIGN - used by BufTableAssign(), similar to HASH_ENTER, but
uses the entry from the single item cache, rather than asking freelist
This last call can fail if someone else already inserted the tag, in
which case it adds the single item cache entry back onto freelist
Notice that single item cache is not in shared memory, so on abort we
should give it back, so we probably need an extra API call for that
also to avoid leaking an entry.
Doing it this way allows us to
* avoid touching freelists altogether in the common path - we know we
are about to reassign the entry, so we do remember it - no contention
from other backends, no borrowing etc..
* avoid sharing the private details outside of the dynahash module
* allows us to use the same technique elsewhere that we have
partitioned hash tables
This approach is cleaner than v1, but should also perform better
because there will be a 1:1 relationship between a buffer and its
dynahash entry, most of the time.
With these changes, I think we will be able to *reduce* the number of
freelists for partitioned dynahash from 32 to maybe 8, as originally
speculated by Robert in 2016:
https://www.postgresql.org/message-id/CA%2BTgmoZkg-04rcNRURt%3DjAG0Cs5oPyB-qKxH4wqX09e-oXy-nw%40mail...
since the freelists will be much less contended with the above approach
It would be useful to see performance with a higher number of connections, >400.
--
Simon Riggs http://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
@ 2022-02-25 09:24 ` Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Yura Sokolov @ 2022-02-25 09:24 UTC (permalink / raw)
To: Simon Riggs <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; Andrey Borodin <[email protected]>; PostgreSQL Developers <[email protected]>
Hello, Simon.
В Пт, 25/02/2022 в 04:35 +0000, Simon Riggs пишет:
> On Mon, 21 Feb 2022 at 08:06, Yura Sokolov <[email protected]> wrote:
> > Good day, Kyotaro Horiguchi and hackers.
> >
> > В Чт, 17/02/2022 в 14:16 +0900, Kyotaro Horiguchi пишет:
> > > At Wed, 16 Feb 2022 10:40:56 +0300, Yura Sokolov <[email protected]> wrote in
> > > > Hello, all.
> > > >
> > > > I thought about patch simplification, and tested version
> > > > without BufTable and dynahash api change at all.
> > > >
> > > > It performs suprisingly well. It is just a bit worse
> > > > than v1 since there is more contention around dynahash's
> > > > freelist, but most of improvement remains.
> > > >
> > > > I'll finish benchmarking and will attach graphs with
> > > > next message. Patch is attached here.
> > >
> > > Thanks for the new patch. The patch as a whole looks fine to me. But
> > > some comments needs to be revised.
> >
> > Thank you for review and remarks.
>
> v3 gets the buffer partition locking right, well done, great results!
>
> In v3, the comment at line 1279 still implies we take both locks
> together, which is not now the case.
>
> Dynahash actions are still possible. You now have the BufTableDelete
> before the BufTableInsert, which opens up the possibility I discussed
> here:
> http://postgr.es/m/CANbhV-F0H-8oB_A+m=55hP0e0QRL=RdDDQuSXMTFt6JPrdX+pQ@mail.gmail.com
> (Apologies for raising a similar topic, I hadn't noticed this thread
> before; thanks to Horiguchi-san for pointing this out).
>
> v1 had a horrible API (sorry!) where you returned the entry and then
> explicitly re-used it. I think we *should* make changes to dynahash,
> but not with the API you proposed.
>
> Proposal for new BufTable API
> BufTableReuse() - similar to BufTableDelete() but does NOT put entry
> back on freelist, we remember it in a private single item cache in
> dynahash
> BufTableAssign() - similar to BufTableInsert() but can only be
> executed directly after BufTableReuse(), fails with ERROR otherwise.
> Takes the entry from single item cache and re-assigns it to new tag
>
> In dynahash we have two new modes that match the above
> HASH_REUSE - used by BufTableReuse(), similar to HASH_REMOVE, but
> places entry on the single item cache, avoiding freelist
> HASH_ASSIGN - used by BufTableAssign(), similar to HASH_ENTER, but
> uses the entry from the single item cache, rather than asking freelist
> This last call can fail if someone else already inserted the tag, in
> which case it adds the single item cache entry back onto freelist
>
> Notice that single item cache is not in shared memory, so on abort we
> should give it back, so we probably need an extra API call for that
> also to avoid leaking an entry.
Why there is need for this? Which way backend could be forced to abort
between BufTableReuse and BufTableAssign in this code path? I don't
see any CHECK_FOR_INTERRUPTS on the way, but may be I'm missing
something.
>
> Doing it this way allows us to
> * avoid touching freelists altogether in the common path - we know we
> are about to reassign the entry, so we do remember it - no contention
> from other backends, no borrowing etc..
> * avoid sharing the private details outside of the dynahash module
> * allows us to use the same technique elsewhere that we have
> partitioned hash tables
>
> This approach is cleaner than v1, but should also perform better
> because there will be a 1:1 relationship between a buffer and its
> dynahash entry, most of the time.
Thank you for suggestion. Yes, it is much clearer than my initial proposal.
Should I incorporate it to v4 patch? Perhaps, it could be a separate
commit in new version.
>
> With these changes, I think we will be able to *reduce* the number of
> freelists for partitioned dynahash from 32 to maybe 8, as originally
> speculated by Robert in 2016:
> https://www.postgresql.org/message-id/CA%2BTgmoZkg-04rcNRURt%3DjAG0Cs5oPyB-qKxH4wqX09e-oXy-nw%40mail...
> since the freelists will be much less contended with the above approach
>
> It would be useful to see performance with a higher number of connections, >400.
>
> --
> Simon Riggs http://www.EnterpriseDB.com/
------
regards,
Yura Sokolov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-02-25 09:38 ` Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Simon Riggs @ 2022-02-25 09:38 UTC (permalink / raw)
To: Yura Sokolov <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; Andrey Borodin <[email protected]>; PostgreSQL Developers <[email protected]>
On Fri, 25 Feb 2022 at 09:24, Yura Sokolov <[email protected]> wrote:
> > This approach is cleaner than v1, but should also perform better
> > because there will be a 1:1 relationship between a buffer and its
> > dynahash entry, most of the time.
>
> Thank you for suggestion. Yes, it is much clearer than my initial proposal.
>
> Should I incorporate it to v4 patch? Perhaps, it could be a separate
> commit in new version.
I don't insist that you do that, but since the API changes are a few
hours work ISTM better to include in one patch for combined perf
testing. It would be better to put all changes in this area into PG15
than to split it across multiple releases.
> Why there is need for this? Which way backend could be forced to abort
> between BufTableReuse and BufTableAssign in this code path? I don't
> see any CHECK_FOR_INTERRUPTS on the way, but may be I'm missing
> something.
Sounds reasonable.
--
Simon Riggs http://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
@ 2022-03-01 07:24 ` Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Yura Sokolov @ 2022-03-01 07:24 UTC (permalink / raw)
To: Simon Riggs <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; Andrey Borodin <[email protected]>; PostgreSQL Developers <[email protected]>; Andres Freund <[email protected]>
В Пт, 25/02/2022 в 09:38 +0000, Simon Riggs пишет:
> On Fri, 25 Feb 2022 at 09:24, Yura Sokolov <[email protected]> wrote:
>
> > > This approach is cleaner than v1, but should also perform better
> > > because there will be a 1:1 relationship between a buffer and its
> > > dynahash entry, most of the time.
> >
> > Thank you for suggestion. Yes, it is much clearer than my initial proposal.
> >
> > Should I incorporate it to v4 patch? Perhaps, it could be a separate
> > commit in new version.
>
> I don't insist that you do that, but since the API changes are a few
> hours work ISTM better to include in one patch for combined perf
> testing. It would be better to put all changes in this area into PG15
> than to split it across multiple releases.
>
> > Why there is need for this? Which way backend could be forced to abort
> > between BufTableReuse and BufTableAssign in this code path? I don't
> > see any CHECK_FOR_INTERRUPTS on the way, but may be I'm missing
> > something.
>
> Sounds reasonable.
Ok, here is v4.
It is with two commits: one for BufferAlloc locking change and other
for dynahash's freelist avoiding.
Buffer locking patch is same to v2 with some comment changes. Ie it uses
Lock+UnlockBufHdr
For dynahash HASH_REUSE and HASH_ASSIGN as suggested.
HASH_REUSE stores deleted element into per-process static variable.
HASH_ASSIGN uses this element instead of freelist. If there's no
such stored element, it falls back to HASH_ENTER.
I've implemented Robert Haas's suggestion to count element in freelists
instead of nentries:
> One idea is to jigger things so that we maintain a count of the total
> number of entries that doesn't change except when we allocate, and
> then for each freelist partition we maintain the number of entries in
> that freelist partition. So then the size of the hash table, instead
> of being sum(nentries) is totalsize - sum(nfree).
https://postgr.es/m/CA%2BTgmoZkg-04rcNRURt%3DjAG0Cs5oPyB-qKxH4wqX09e-oXy-nw%40mail.gmail.com
It helps to avoid freelist lock just to actualize counters.
I made it with replacing "nentries" with "nfree" and adding
"nalloced" to each freelist. It also makes "hash_update_hash_key" valid
for key that migrates partitions.
I believe, there is no need for "nalloced" for each freelist, and
instead single such field should be in HASHHDR. More, it seems to me
`element_alloc` function needs no acquiring freelist partition lock
since it is called only during initialization of shared hash table.
Am I right?
I didn't go this path in v4 for simplicity, but can put it to v5
if approved.
To be honest, "reuse" patch gives little improvement. But still
measurable on some connection numbers.
I tried to reduce freelist partitions to 8, but it has mixed impact.
Most of time performance is same, but sometimes a bit lower. I
didn't investigate reasons. Perhaps they are not related to buffer
manager.
I didn't introduce new functions BufTableReuse and BufTableAssign
since there are single call to BufTableInsert and two calls to
BufTableDelete. So I reused this functions, just added "reuse" flag
to BufTableDelete.
Tests simple_select for Xeon 8354H, 128MB and 1G shared buffers
for scale 100.
1 socket:
conns | master | patch_v4 | master 1G | patch_v4 1G
--------+------------+------------+------------+------------
1 | 41975 | 41540 | 52898 | 52213
2 | 77693 | 77908 | 97571 | 98371
3 | 114713 | 115522 | 142709 | 145226
5 | 188898 | 187617 | 239322 | 237269
7 | 261516 | 260006 | 329119 | 329449
17 | 521821 | 519473 | 672390 | 662106
27 | 555487 | 555697 | 674630 | 672736
53 | 868213 | 896539 | 1190734 | 1202505
83 | 868232 | 866029 | 1164997 | 1158719
107 | 850477 | 845685 | 1140597 | 1134502
139 | 816311 | 816808 | 1101471 | 1091258
163 | 794788 | 796517 | 1078445 | 1071568
191 | 765934 | 776185 | 1059497 | 1041944
211 | 738656 | 777365 | 1083356 | 1046422
239 | 713124 | 841337 | 1104629 | 1116668
271 | 692138 | 847803 | 1094432 | 1128971
307 | 682919 | 849239 | 1086306 | 1127051
353 | 679449 | 842125 | 1071482 | 1117471
397 | 676217 | 844015 | 1058937 | 1118628
2 sockets:
conns | master | patch_v4 | master 1G | patch_v4 1G
--------+------------+------------+------------+------------
1 | 44317 | 44034 | 53920 | 53583
2 | 81193 | 78621 | 99138 | 97968
3 | 120755 | 115648 | 148102 | 147423
5 | 190007 | 188943 | 232078 | 231029
7 | 258602 | 260649 | 325545 | 318567
17 | 551814 | 552914 | 692312 | 697518
27 | 787353 | 786573 | 1023509 | 1022891
53 | 973880 | 1008534 | 1228274 | 1278194
83 | 1108442 | 1269777 | 1596292 | 1648156
107 | 1072188 | 1339634 | 1542401 | 1664476
139 | 1000446 | 1316372 | 1490757 | 1676127
163 | 967378 | 1257445 | 1461468 | 1655574
191 | 926010 | 1189591 | 1435317 | 1639313
211 | 909919 | 1149905 | 1417437 | 1632764
239 | 895944 | 1115681 | 1393530 | 1616329
271 | 880545 | 1090208 | 1374878 | 1609544
307 | 865560 | 1066798 | 1355164 | 1593769
353 | 857591 | 1046426 | 1330069 | 1584006
397 | 840374 | 1024711 | 1312257 | 1564872
--------
regards
Yura Sokolov
Postgres Professional
[email protected]
[email protected]
Attachments:
[text/x-patch] v4-0002-PGPRO-5616-Add-HASH_REUSE-HASH_ASSIGN-and-use-it-.patch (14.0K, ../../[email protected]/2-v4-0002-PGPRO-5616-Add-HASH_REUSE-HASH_ASSIGN-and-use-it-.patch)
download | inline diff:
From 22d9613accc70eb2f9e799b87e976d64540f36b4 Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Mon, 28 Feb 2022 12:19:17 +0300
Subject: [PATCH v4 2/2] Add HASH_REUSE+HASH_ASSIGN and use it in BufTable.
Avoid dynahash's freelist locking when BufferAlloc reuses buffer for
different tag.
HASH_REUSE acts as HASH_REMOVE, but stores element to reuse in static
variable instead of freelist partition. And HASH_ASSIGN then uses the
element.
Unfortunately, FreeListData->nentries had to be manipulated even in this
case. So instead of manipulation with nentries, we replace nentries with
nfree - actual length of free list, and nalloced - initially allocated
entries for free list. This were suggested by Robert Haas in
https://postgr.es/m/CA%2BTgmoZkg-04rcNRURt%3DjAG0Cs5oPyB-qKxH4wqX09e-oXy-nw%40mail.gmail.com
---
src/backend/storage/buffer/buf_table.c | 9 +-
src/backend/storage/buffer/bufmgr.c | 4 +-
src/backend/utils/hash/dynahash.c | 130 ++++++++++++++++++++-----
src/include/storage/buf_internals.h | 2 +-
src/include/utils/hsearch.h | 4 +-
5 files changed, 120 insertions(+), 29 deletions(-)
diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c
index caa03ae1233..3362c7127e9 100644
--- a/src/backend/storage/buffer/buf_table.c
+++ b/src/backend/storage/buffer/buf_table.c
@@ -128,7 +128,7 @@ BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id)
hash_search_with_hash_value(SharedBufHash,
(void *) tagPtr,
hashcode,
- HASH_ENTER,
+ HASH_ASSIGN,
&found);
if (found) /* found something already in the table */
@@ -143,10 +143,13 @@ BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id)
* BufTableDelete
* Delete the hashtable entry for given tag (which must exist)
*
+ * If reuse flag is true, deleted entry is cached for reuse, and caller
+ * must call BufTableInsert next.
+ *
* Caller must hold exclusive lock on BufMappingLock for tag's partition
*/
void
-BufTableDelete(BufferTag *tagPtr, uint32 hashcode)
+BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse)
{
BufferLookupEnt *result;
@@ -154,7 +157,7 @@ BufTableDelete(BufferTag *tagPtr, uint32 hashcode)
hash_search_with_hash_value(SharedBufHash,
(void *) tagPtr,
hashcode,
- HASH_REMOVE,
+ reuse ? HASH_REUSE : HASH_REMOVE,
NULL);
if (!result) /* shouldn't happen */
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 5d2781f4813..85b62463c0d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1334,7 +1334,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* Delete old tag from hash table if it were valid. */
if (oldFlags & BM_TAG_VALID)
- BufTableDelete(&oldTag, oldHash);
+ BufTableDelete(&oldTag, oldHash, true);
if (oldPartitionLock != newPartitionLock)
{
@@ -1528,7 +1528,7 @@ retry:
* Remove the buffer from the lookup hashtable, if it was in there.
*/
if (oldFlags & BM_TAG_VALID)
- BufTableDelete(&oldTag, oldHash);
+ BufTableDelete(&oldTag, oldHash, false);
/*
* Done with mapping lock.
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 6546e3c7c79..9eb07593da7 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -14,7 +14,7 @@
* a hash table in partitioned mode, the HASH_PARTITION flag must be given
* to hash_create. This prevents any attempt to split buckets on-the-fly.
* Therefore, each hash bucket chain operates independently, and no fields
- * of the hash header change after init except nentries and freeList.
+ * of the hash header change after init except nfree and freeList.
* (A partitioned table uses multiple copies of those fields, guarded by
* spinlocks, for additional concurrency.)
* This lets any subset of the hash buckets be treated as a separately
@@ -138,8 +138,9 @@ typedef HASHBUCKET *HASHSEGMENT;
*
* In a partitioned hash table, each freelist is associated with a specific
* set of hashcodes, as determined by the FREELIST_IDX() macro below.
- * nentries tracks the number of live hashtable entries having those hashcodes
- * (NOT the number of entries in the freelist, as you might expect).
+ * nalloced tracks the number of free hashtable entries initially allocated
+ * for the freelist.
+ * nfree tracks the actual number of free hashtable entries in the freelist.
*
* The coverage of a freelist might be more or less than one partition, so it
* needs its own lock rather than relying on caller locking. Relying on that
@@ -147,13 +148,15 @@ typedef HASHBUCKET *HASHSEGMENT;
* need to "borrow" entries from another freelist; see get_hash_entry().
*
* Using an array of FreeListData instead of separate arrays of mutexes,
- * nentries and freeLists helps to reduce sharing of cache lines between
+ * nfree and freeLists helps to reduce sharing of cache lines between
* different mutexes.
*/
typedef struct
{
slock_t mutex; /* spinlock for this freelist */
- long nentries; /* number of entries in associated buckets */
+ long nfree; /* number of free entries in the list */
+ long nalloced; /* number of entries initially allocated for
+ * the list */
HASHELEMENT *freeList; /* chain of free elements */
} FreeListData;
@@ -170,7 +173,7 @@ struct HASHHDR
/*
* The freelist can become a point of contention in high-concurrency hash
* tables, so we use an array of freelists, each with its own mutex and
- * nentries count, instead of just a single one. Although the freelists
+ * nfree count, instead of just a single one. Although the freelists
* normally operate independently, we will scavenge entries from freelists
* other than a hashcode's default freelist when necessary.
*
@@ -254,6 +257,15 @@ struct HTAB
*/
#define MOD(x,y) ((x) & ((y)-1))
+/*
+ * Struct for reuse element.
+ */
+struct HASHREUSE
+{
+ HTAB *hashp;
+ HASHBUCKET element;
+};
+
#ifdef HASH_STATISTICS
static long hash_accesses,
hash_collisions,
@@ -293,6 +305,12 @@ DynaHashAlloc(Size size)
}
+/*
+ * Support for HASH_REUSE + HASH_ASSIGN
+ */
+static struct HASHREUSE DynaHashReuse = {NULL, NULL};
+
+
/*
* HashCompareFunc for string keys
*
@@ -932,6 +950,10 @@ calc_bucket(HASHHDR *hctl, uint32 hash_val)
* HASH_ENTER: look up key in table, creating entry if not present
* HASH_ENTER_NULL: same, but return NULL if out of memory
* HASH_REMOVE: look up key in table, remove entry if present
+ * HASH_REUSE: same as HASH_REMOVE, but stores removed element in static
+ * variable instead of free list.
+ * HASH_ASSIGN: same as HASH_ENTER, but reuses element stored by HASH_REUSE
+ * if any.
*
* Return value is a pointer to the element found/entered/removed if any,
* or NULL if no match was found. (NB: in the case of the REMOVE action,
@@ -1000,7 +1022,8 @@ hash_search_with_hash_value(HTAB *hashp,
* Can't split if running in partitioned mode, nor if frozen, nor if
* table is the subject of any active hash_seq_search scans.
*/
- if (hctl->freeList[0].nentries > (long) hctl->max_bucket &&
+ if (hctl->freeList[0].nfree == 0 &&
+ hctl->freeList[0].nalloced > (long) hctl->max_bucket &&
!IS_PARTITIONED(hctl) && !hashp->frozen &&
!has_seq_scans(hashp))
(void) expand_table(hashp);
@@ -1044,6 +1067,10 @@ hash_search_with_hash_value(HTAB *hashp,
if (foundPtr)
*foundPtr = (bool) (currBucket != NULL);
+ /* If HASH_REUSE were not called, HASH_ASSIGN falls back to HASH_ENTER */
+ if (action == HASH_ASSIGN && DynaHashReuse.element == NULL)
+ action = HASH_ENTER;
+
/*
* OK, now what?
*/
@@ -1057,20 +1084,17 @@ hash_search_with_hash_value(HTAB *hashp,
case HASH_REMOVE:
if (currBucket != NULL)
{
- /* if partitioned, must lock to touch nentries and freeList */
+ /* if partitioned, must lock to touch nfree and freeList */
if (IS_PARTITIONED(hctl))
SpinLockAcquire(&(hctl->freeList[freelist_idx].mutex));
- /* delete the record from the appropriate nentries counter. */
- Assert(hctl->freeList[freelist_idx].nentries > 0);
- hctl->freeList[freelist_idx].nentries--;
-
/* remove record from hash bucket's chain. */
*prevBucketPtr = currBucket->link;
/* add the record to the appropriate freelist. */
currBucket->link = hctl->freeList[freelist_idx].freeList;
hctl->freeList[freelist_idx].freeList = currBucket;
+ hctl->freeList[freelist_idx].nfree++;
if (IS_PARTITIONED(hctl))
SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
@@ -1116,6 +1140,7 @@ hash_search_with_hash_value(HTAB *hashp,
errmsg("out of memory")));
}
+ link_element:
/* link into hashbucket chain */
*prevBucketPtr = currBucket;
currBucket->link = NULL;
@@ -1132,6 +1157,63 @@ hash_search_with_hash_value(HTAB *hashp,
*/
return (void *) ELEMENTKEY(currBucket);
+
+ case HASH_REUSE:
+ if (currBucket != NULL)
+ {
+ /* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
+ Assert(DynaHashReuseHTAB == NULL);
+ Assert(DynaHashReuseElement == NULL);
+
+ /* remove record from hash bucket's chain. */
+ *prevBucketPtr = currBucket->link;
+
+ /* and store for HASH_ASSIGN */
+ DynaHashReuse.element = currBucket;
+ DynaHashReuse.hashp = hashp;
+
+ /* Caller should call HASH_ASSIGN as the very next step. */
+ return (void *) ELEMENTKEY(currBucket);
+ }
+ return NULL;
+
+ case HASH_ASSIGN:
+ /* check HASH_REUSE were called for same hash table */
+ Assert(DynaHashReuse.hashp == hashp);
+
+ /*
+ * If existing element is found, need to put Reuse element to
+ * original freelist. There is no much difference, which list to
+ * put, since we migrate buckets between buckets.
+ */
+ if (currBucket != NULL)
+ {
+
+ /* if partitioned, must lock to touch nfree and freeList */
+ if (IS_PARTITIONED(hctl))
+ SpinLockAcquire(&(hctl->freeList[freelist_idx].mutex));
+
+ /* add the record to the appropriate freelist. */
+ DynaHashReuse.element->link = hctl->freeList[freelist_idx].freeList;
+ hctl->freeList[freelist_idx].freeList = DynaHashReuse.element;
+ hctl->freeList[freelist_idx].nfree++;
+
+ if (IS_PARTITIONED(hctl))
+ SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
+
+ DynaHashReuse.element = NULL;
+ DynaHashReuse.hashp = NULL;
+
+ return (void *) ELEMENTKEY(currBucket);
+ }
+
+ currBucket = DynaHashReuse.element;
+
+ DynaHashReuse.element = NULL;
+ DynaHashReuse.hashp = NULL;
+
+ /* reuse HASH_ENTER code */
+ goto link_element;
}
elog(ERROR, "unrecognized hash action code: %d", (int) action);
@@ -1301,7 +1383,7 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
for (;;)
{
- /* if partitioned, must lock to touch nentries and freeList */
+ /* if partitioned, must lock to touch nfree and freeList */
if (IS_PARTITIONED(hctl))
SpinLockAcquire(&hctl->freeList[freelist_idx].mutex);
@@ -1347,13 +1429,9 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
if (newElement != NULL)
{
hctl->freeList[borrow_from_idx].freeList = newElement->link;
+ hctl->freeList[borrow_from_idx].nfree--;
SpinLockRelease(&(hctl->freeList[borrow_from_idx].mutex));
- /* careful: count the new element in its proper freelist */
- SpinLockAcquire(&hctl->freeList[freelist_idx].mutex);
- hctl->freeList[freelist_idx].nentries++;
- SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
-
return newElement;
}
@@ -1365,9 +1443,9 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
}
}
- /* remove entry from freelist, bump nentries */
+ /* remove entry from freelist, decrease nfree */
hctl->freeList[freelist_idx].freeList = newElement->link;
- hctl->freeList[freelist_idx].nentries++;
+ hctl->freeList[freelist_idx].nfree--;
if (IS_PARTITIONED(hctl))
SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
@@ -1382,7 +1460,10 @@ long
hash_get_num_entries(HTAB *hashp)
{
int i;
- long sum = hashp->hctl->freeList[0].nentries;
+ long sum = 0;
+
+ sum += hashp->hctl->freeList[0].nalloced;
+ sum -= hashp->hctl->freeList[0].nfree;
/*
* We currently don't bother with acquiring the mutexes; it's only
@@ -1392,7 +1473,10 @@ hash_get_num_entries(HTAB *hashp)
if (IS_PARTITIONED(hashp->hctl))
{
for (i = 1; i < NUM_FREELISTS; i++)
- sum += hashp->hctl->freeList[i].nentries;
+ {
+ sum += hashp->hctl->freeList[i].nalloced;
+ sum -= hashp->hctl->freeList[i].nfree;
+ }
}
return sum;
@@ -1739,6 +1823,8 @@ element_alloc(HTAB *hashp, int nelem, int freelist_idx)
/* freelist could be nonempty if two backends did this concurrently */
firstElement->link = hctl->freeList[freelist_idx].freeList;
hctl->freeList[freelist_idx].freeList = prevElement;
+ hctl->freeList[freelist_idx].nfree += nelem;
+ hctl->freeList[freelist_idx].nalloced += nelem;
if (IS_PARTITIONED(hctl))
SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 7c6653311a5..d35ee1b4108 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -328,7 +328,7 @@ extern void InitBufTable(int size);
extern uint32 BufTableHashCode(BufferTag *tagPtr);
extern int BufTableLookup(BufferTag *tagPtr, uint32 hashcode);
extern int BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id);
-extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode);
+extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse);
/* localbuf.c */
extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h
index d7af0239c8c..ba21c5a65a1 100644
--- a/src/include/utils/hsearch.h
+++ b/src/include/utils/hsearch.h
@@ -113,7 +113,9 @@ typedef enum
HASH_FIND,
HASH_ENTER,
HASH_REMOVE,
- HASH_ENTER_NULL
+ HASH_ENTER_NULL,
+ HASH_REUSE,
+ HASH_ASSIGN
} HASHACTION;
/* hash_seq status (should be considered an opaque type by callers) */
--
2.35.1
[text/x-patch] v4-0001-bufmgr-do-not-acquire-two-partition-lo.patch (8.5K, ../../[email protected]/3-v4-0001-bufmgr-do-not-acquire-two-partition-lo.patch)
download | inline diff:
From c1b8e6d60030d5d02287ae731ab604feeafa7486 Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Mon, 21 Feb 2022 08:49:03 +0300
Subject: [PATCH v4 1/2] bufmgr: do not acquire two partition locks.
Acquiring two partition locks leads to complex dependency chain that hurts
at high concurrency level.
There is no need to hold both lock simultaneously. Buffer is pinned so
other processes could not select it for eviction. If tag is cleared and
buffer removed from old partition other processes will not find it.
Therefore it is safe to release old partition lock before acquiring
new partition lock.
---
src/backend/storage/buffer/bufmgr.c | 189 +++++++++++++---------------
1 file changed, 89 insertions(+), 100 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b4532948d3f..5d2781f4813 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1288,93 +1288,16 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
oldHash = BufTableHashCode(&oldTag);
oldPartitionLock = BufMappingPartitionLock(oldHash);
- /*
- * Must lock the lower-numbered partition first to avoid
- * deadlocks.
- */
- if (oldPartitionLock < newPartitionLock)
- {
- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- }
- else if (oldPartitionLock > newPartitionLock)
- {
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
- }
- else
- {
- /* only one partition, only one lock */
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- }
+ LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
}
else
{
- /* if it wasn't valid, we need only the new partition */
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
/* remember we have no old-partition lock or tag */
oldPartitionLock = NULL;
/* keep the compiler quiet about uninitialized variables */
oldHash = 0;
}
- /*
- * Try to make a hashtable entry for the buffer under its new tag.
- * This could fail because while we were writing someone else
- * allocated another buffer for the same block we want to read in.
- * Note that we have not yet removed the hashtable entry for the old
- * tag.
- */
- buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
-
- if (buf_id >= 0)
- {
- /*
- * Got a collision. Someone has already done what we were about to
- * do. We'll just handle this as if it were found in the buffer
- * pool in the first place. First, give up the buffer we were
- * planning to use.
- */
- UnpinBuffer(buf, true);
-
- /* Can give up that buffer's mapping partition lock now */
- if (oldPartitionLock != NULL &&
- oldPartitionLock != newPartitionLock)
- LWLockRelease(oldPartitionLock);
-
- /* remaining code should match code at top of routine */
-
- buf = GetBufferDescriptor(buf_id);
-
- valid = PinBuffer(buf, strategy);
-
- /* Can release the mapping lock as soon as we've pinned it */
- LWLockRelease(newPartitionLock);
-
- *foundPtr = true;
-
- if (!valid)
- {
- /*
- * We can only get here if (a) someone else is still reading
- * in the page, or (b) a previous read attempt failed. We
- * have to wait for any active read attempt to finish, and
- * then set up our own read attempt if the page is still not
- * BM_VALID. StartBufferIO does it all.
- */
- if (StartBufferIO(buf, true))
- {
- /*
- * If we get here, previous attempts to read the buffer
- * must have failed ... but we shall bravely try again.
- */
- *foundPtr = false;
- }
- }
-
- return buf;
- }
-
/*
* Need to lock the buffer header too in order to change its tag.
*/
@@ -1382,40 +1305,113 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/*
* Somebody could have pinned or re-dirtied the buffer while we were
- * doing the I/O and making the new hashtable entry. If so, we can't
- * recycle this buffer; we must undo everything we've done and start
- * over with a new victim buffer.
+ * doing the I/O. If so, we can't recycle this buffer; we must undo
+ * everything we've done and start over with a new victim buffer.
*/
oldFlags = buf_state & BUF_FLAG_MASK;
if (BUF_STATE_GET_REFCOUNT(buf_state) == 1 && !(oldFlags & BM_DIRTY))
break;
UnlockBufHdr(buf, buf_state);
- BufTableDelete(&newTag, newHash);
- if (oldPartitionLock != NULL &&
- oldPartitionLock != newPartitionLock)
+ if (oldPartitionLock != NULL)
LWLockRelease(oldPartitionLock);
- LWLockRelease(newPartitionLock);
UnpinBuffer(buf, true);
}
/*
- * Okay, it's finally safe to rename the buffer.
+ * Clear out the buffer's tag and flags. We must do this to ensure that
+ * linear scans of the buffer array don't think the buffer is valid. We
+ * also reset the usage_count since any recency of use of the old content
+ * is no longer relevant.
*
- * Clearing BM_VALID here is necessary, clearing the dirtybits is just
- * paranoia. We also reset the usage_count since any recency of use of
- * the old content is no longer relevant. (The usage_count starts out at
- * 1 so that the buffer can survive one clock-sweep pass.)
+ * We are single pinner, we hold buffer header lock and exclusive
+ * partition lock (if tag is valid). Given these statements it is safe to
+ * clear tag since no other process can inspect it to the moment.
+ */
+ CLEAR_BUFFERTAG(buf->tag);
+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
+ UnlockBufHdr(buf, buf_state);
+
+ /* Delete old tag from hash table if it were valid. */
+ if (oldFlags & BM_TAG_VALID)
+ BufTableDelete(&oldTag, oldHash);
+
+ if (oldPartitionLock != newPartitionLock)
+ {
+ if (oldPartitionLock != NULL)
+ LWLockRelease(oldPartitionLock);
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ }
+
+ /*
+ * Try to make a hashtable entry for the buffer under its new tag. This
+ * could fail because while we were writing someone else allocated another
+ * buffer for the same block we want to read in. In that case we will have
+ * to return our buffer to free list.
+ */
+ buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
+
+ if (buf_id >= 0)
+ {
+ /*
+ * Got a collision. Someone has already done what we were about to do.
+ * We'll just handle this as if it were found in the buffer pool in
+ * the first place.
+ */
+
+ /*
+ * First, give up the buffer we were planning to use and put it to
+ * free lists.
+ */
+ UnpinBuffer(buf, true);
+ StrategyFreeBuffer(buf);
+
+ /* remaining code should match code at top of routine */
+
+ buf = GetBufferDescriptor(buf_id);
+
+ valid = PinBuffer(buf, strategy);
+
+ /* Can release the mapping lock as soon as we've pinned it */
+ LWLockRelease(newPartitionLock);
+
+ *foundPtr = true;
+
+ if (!valid)
+ {
+ /*
+ * We can only get here if (a) someone else is still reading in
+ * the page, or (b) a previous read attempt failed. We have to
+ * wait for any active read attempt to finish, and then set up our
+ * own read attempt if the page is still not BM_VALID.
+ * StartBufferIO does it all.
+ */
+ if (StartBufferIO(buf, true))
+ {
+ /*
+ * If we get here, previous attempts to read the buffer must
+ * have failed ... but we shall bravely try again.
+ */
+ *foundPtr = false;
+ }
+ }
+
+ return buf;
+ }
+
+ /*
+ * Now it is safe to use victim buffer for new tag.
*
* Make sure BM_PERMANENT is set for buffers that must be written at every
* checkpoint. Unlogged buffers only need to be written at shutdown
* checkpoints, except for their "init" forks, which need to be treated
* just like permanent relations.
+ *
+ * The usage_count starts out at 1 so that the buffer can survive one
+ * clock-sweep pass.
*/
+ buf_state = LockBufHdr(buf);
buf->tag = newTag;
- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
- BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
- BUF_USAGECOUNT_MASK);
if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
else
@@ -1423,13 +1419,6 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
UnlockBufHdr(buf, buf_state);
- if (oldPartitionLock != NULL)
- {
- BufTableDelete(&oldTag, oldHash);
- if (oldPartitionLock != newPartitionLock)
- LWLockRelease(oldPartitionLock);
- }
-
LWLockRelease(newPartitionLock);
/*
--
2.35.1
[image/gif] v4-1socket.gif (13.4K, ../../[email protected]/4-v4-1socket.gif)
download | view image
[image/gif] v4-2socket.gif (13.9K, ../../[email protected]/5-v4-2socket.gif)
download | view image
[application/zip] res.zip (47.5K, ../../[email protected]/6-res.zip)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-03-02 22:35 ` Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Yura Sokolov @ 2022-03-02 22:35 UTC (permalink / raw)
To: PostgreSQL Developers <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Simon Riggs <[email protected]>
В Вт, 01/03/2022 в 10:24 +0300, Yura Sokolov пишет:
> Ok, here is v4.
And here is v5.
First, there was compilation error in Assert in dynahash.c .
Excuse me for not checking before sending previous version.
Second, I add third commit that reduces HASHHDR allocation
size for non-partitioned dynahash:
- moved freeList to last position
- alloc and memset offset(HASHHDR, freeList[1]) for
non-partitioned hash tables.
I didn't benchmarked it, but I will be surprised if it
matters much in performance sence.
Third, I put all three commits into single file to not
confuse commitfest application.
--------
regards
Yura Sokolov
Postgres Professional
[email protected]
[email protected]
Attachments:
[text/x-patch] v5-bufmgr-lock-improvements.patch (26.0K, ../../[email protected]/2-v5-bufmgr-lock-improvements.patch)
download | inline diff:
From c1b8e6d60030d5d02287ae731ab604feeafa7486 Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Mon, 21 Feb 2022 08:49:03 +0300
Subject: [PATCH 1/3] bufmgr: do not acquire two partition locks.
Acquiring two partition locks leads to complex dependency chain that hurts
at high concurrency level.
There is no need to hold both lock simultaneously. Buffer is pinned so
other processes could not select it for eviction. If tag is cleared and
buffer removed from old partition other processes will not find it.
Therefore it is safe to release old partition lock before acquiring
new partition lock.
---
src/backend/storage/buffer/bufmgr.c | 189 +++++++++++++---------------
1 file changed, 89 insertions(+), 100 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b4532948d3f..5d2781f4813 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1288,93 +1288,16 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
oldHash = BufTableHashCode(&oldTag);
oldPartitionLock = BufMappingPartitionLock(oldHash);
- /*
- * Must lock the lower-numbered partition first to avoid
- * deadlocks.
- */
- if (oldPartitionLock < newPartitionLock)
- {
- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- }
- else if (oldPartitionLock > newPartitionLock)
- {
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
- }
- else
- {
- /* only one partition, only one lock */
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
- }
+ LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
}
else
{
- /* if it wasn't valid, we need only the new partition */
- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
/* remember we have no old-partition lock or tag */
oldPartitionLock = NULL;
/* keep the compiler quiet about uninitialized variables */
oldHash = 0;
}
- /*
- * Try to make a hashtable entry for the buffer under its new tag.
- * This could fail because while we were writing someone else
- * allocated another buffer for the same block we want to read in.
- * Note that we have not yet removed the hashtable entry for the old
- * tag.
- */
- buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
-
- if (buf_id >= 0)
- {
- /*
- * Got a collision. Someone has already done what we were about to
- * do. We'll just handle this as if it were found in the buffer
- * pool in the first place. First, give up the buffer we were
- * planning to use.
- */
- UnpinBuffer(buf, true);
-
- /* Can give up that buffer's mapping partition lock now */
- if (oldPartitionLock != NULL &&
- oldPartitionLock != newPartitionLock)
- LWLockRelease(oldPartitionLock);
-
- /* remaining code should match code at top of routine */
-
- buf = GetBufferDescriptor(buf_id);
-
- valid = PinBuffer(buf, strategy);
-
- /* Can release the mapping lock as soon as we've pinned it */
- LWLockRelease(newPartitionLock);
-
- *foundPtr = true;
-
- if (!valid)
- {
- /*
- * We can only get here if (a) someone else is still reading
- * in the page, or (b) a previous read attempt failed. We
- * have to wait for any active read attempt to finish, and
- * then set up our own read attempt if the page is still not
- * BM_VALID. StartBufferIO does it all.
- */
- if (StartBufferIO(buf, true))
- {
- /*
- * If we get here, previous attempts to read the buffer
- * must have failed ... but we shall bravely try again.
- */
- *foundPtr = false;
- }
- }
-
- return buf;
- }
-
/*
* Need to lock the buffer header too in order to change its tag.
*/
@@ -1382,40 +1305,113 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/*
* Somebody could have pinned or re-dirtied the buffer while we were
- * doing the I/O and making the new hashtable entry. If so, we can't
- * recycle this buffer; we must undo everything we've done and start
- * over with a new victim buffer.
+ * doing the I/O. If so, we can't recycle this buffer; we must undo
+ * everything we've done and start over with a new victim buffer.
*/
oldFlags = buf_state & BUF_FLAG_MASK;
if (BUF_STATE_GET_REFCOUNT(buf_state) == 1 && !(oldFlags & BM_DIRTY))
break;
UnlockBufHdr(buf, buf_state);
- BufTableDelete(&newTag, newHash);
- if (oldPartitionLock != NULL &&
- oldPartitionLock != newPartitionLock)
+ if (oldPartitionLock != NULL)
LWLockRelease(oldPartitionLock);
- LWLockRelease(newPartitionLock);
UnpinBuffer(buf, true);
}
/*
- * Okay, it's finally safe to rename the buffer.
+ * Clear out the buffer's tag and flags. We must do this to ensure that
+ * linear scans of the buffer array don't think the buffer is valid. We
+ * also reset the usage_count since any recency of use of the old content
+ * is no longer relevant.
*
- * Clearing BM_VALID here is necessary, clearing the dirtybits is just
- * paranoia. We also reset the usage_count since any recency of use of
- * the old content is no longer relevant. (The usage_count starts out at
- * 1 so that the buffer can survive one clock-sweep pass.)
+ * We are single pinner, we hold buffer header lock and exclusive
+ * partition lock (if tag is valid). Given these statements it is safe to
+ * clear tag since no other process can inspect it to the moment.
+ */
+ CLEAR_BUFFERTAG(buf->tag);
+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
+ UnlockBufHdr(buf, buf_state);
+
+ /* Delete old tag from hash table if it were valid. */
+ if (oldFlags & BM_TAG_VALID)
+ BufTableDelete(&oldTag, oldHash);
+
+ if (oldPartitionLock != newPartitionLock)
+ {
+ if (oldPartitionLock != NULL)
+ LWLockRelease(oldPartitionLock);
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ }
+
+ /*
+ * Try to make a hashtable entry for the buffer under its new tag. This
+ * could fail because while we were writing someone else allocated another
+ * buffer for the same block we want to read in. In that case we will have
+ * to return our buffer to free list.
+ */
+ buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
+
+ if (buf_id >= 0)
+ {
+ /*
+ * Got a collision. Someone has already done what we were about to do.
+ * We'll just handle this as if it were found in the buffer pool in
+ * the first place.
+ */
+
+ /*
+ * First, give up the buffer we were planning to use and put it to
+ * free lists.
+ */
+ UnpinBuffer(buf, true);
+ StrategyFreeBuffer(buf);
+
+ /* remaining code should match code at top of routine */
+
+ buf = GetBufferDescriptor(buf_id);
+
+ valid = PinBuffer(buf, strategy);
+
+ /* Can release the mapping lock as soon as we've pinned it */
+ LWLockRelease(newPartitionLock);
+
+ *foundPtr = true;
+
+ if (!valid)
+ {
+ /*
+ * We can only get here if (a) someone else is still reading in
+ * the page, or (b) a previous read attempt failed. We have to
+ * wait for any active read attempt to finish, and then set up our
+ * own read attempt if the page is still not BM_VALID.
+ * StartBufferIO does it all.
+ */
+ if (StartBufferIO(buf, true))
+ {
+ /*
+ * If we get here, previous attempts to read the buffer must
+ * have failed ... but we shall bravely try again.
+ */
+ *foundPtr = false;
+ }
+ }
+
+ return buf;
+ }
+
+ /*
+ * Now it is safe to use victim buffer for new tag.
*
* Make sure BM_PERMANENT is set for buffers that must be written at every
* checkpoint. Unlogged buffers only need to be written at shutdown
* checkpoints, except for their "init" forks, which need to be treated
* just like permanent relations.
+ *
+ * The usage_count starts out at 1 so that the buffer can survive one
+ * clock-sweep pass.
*/
+ buf_state = LockBufHdr(buf);
buf->tag = newTag;
- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
- BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
- BUF_USAGECOUNT_MASK);
if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
else
@@ -1423,13 +1419,6 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
UnlockBufHdr(buf, buf_state);
- if (oldPartitionLock != NULL)
- {
- BufTableDelete(&oldTag, oldHash);
- if (oldPartitionLock != newPartitionLock)
- LWLockRelease(oldPartitionLock);
- }
-
LWLockRelease(newPartitionLock);
/*
--
2.35.1
From 9879b18d6fc0b6beccc71debac62470e26025f84 Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Mon, 28 Feb 2022 12:19:17 +0300
Subject: [PATCH 2/3] Add HASH_REUSE+HASH_ASSIGN and use it in BufTable.
Avoid dynahash's freelist locking when BufferAlloc reuses buffer for
different tag.
HASH_REUSE acts as HASH_REMOVE, but stores element to reuse in static
variable instead of freelist partition. And HASH_ASSIGN then uses the
element.
Unfortunately, FreeListData->nentries had to be manipulated even in this
case. So instead of manipulation with nentries, we replace nentries with
nfree - actual length of free list, and nalloced - initially allocated
entries for free list. This were suggested by Robert Haas in
https://postgr.es/m/CA%2BTgmoZkg-04rcNRURt%3DjAG0Cs5oPyB-qKxH4wqX09e-oXy-nw%40mail.gmail.com
---
src/backend/storage/buffer/buf_table.c | 9 +-
src/backend/storage/buffer/bufmgr.c | 4 +-
src/backend/utils/hash/dynahash.c | 130 ++++++++++++++++++++-----
src/include/storage/buf_internals.h | 2 +-
src/include/utils/hsearch.h | 4 +-
5 files changed, 120 insertions(+), 29 deletions(-)
diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c
index caa03ae1233..3362c7127e9 100644
--- a/src/backend/storage/buffer/buf_table.c
+++ b/src/backend/storage/buffer/buf_table.c
@@ -128,7 +128,7 @@ BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id)
hash_search_with_hash_value(SharedBufHash,
(void *) tagPtr,
hashcode,
- HASH_ENTER,
+ HASH_ASSIGN,
&found);
if (found) /* found something already in the table */
@@ -143,10 +143,13 @@ BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id)
* BufTableDelete
* Delete the hashtable entry for given tag (which must exist)
*
+ * If reuse flag is true, deleted entry is cached for reuse, and caller
+ * must call BufTableInsert next.
+ *
* Caller must hold exclusive lock on BufMappingLock for tag's partition
*/
void
-BufTableDelete(BufferTag *tagPtr, uint32 hashcode)
+BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse)
{
BufferLookupEnt *result;
@@ -154,7 +157,7 @@ BufTableDelete(BufferTag *tagPtr, uint32 hashcode)
hash_search_with_hash_value(SharedBufHash,
(void *) tagPtr,
hashcode,
- HASH_REMOVE,
+ reuse ? HASH_REUSE : HASH_REMOVE,
NULL);
if (!result) /* shouldn't happen */
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 5d2781f4813..85b62463c0d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1334,7 +1334,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* Delete old tag from hash table if it were valid. */
if (oldFlags & BM_TAG_VALID)
- BufTableDelete(&oldTag, oldHash);
+ BufTableDelete(&oldTag, oldHash, true);
if (oldPartitionLock != newPartitionLock)
{
@@ -1528,7 +1528,7 @@ retry:
* Remove the buffer from the lookup hashtable, if it was in there.
*/
if (oldFlags & BM_TAG_VALID)
- BufTableDelete(&oldTag, oldHash);
+ BufTableDelete(&oldTag, oldHash, false);
/*
* Done with mapping lock.
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 6546e3c7c79..af4196cf194 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -14,7 +14,7 @@
* a hash table in partitioned mode, the HASH_PARTITION flag must be given
* to hash_create. This prevents any attempt to split buckets on-the-fly.
* Therefore, each hash bucket chain operates independently, and no fields
- * of the hash header change after init except nentries and freeList.
+ * of the hash header change after init except nfree and freeList.
* (A partitioned table uses multiple copies of those fields, guarded by
* spinlocks, for additional concurrency.)
* This lets any subset of the hash buckets be treated as a separately
@@ -138,8 +138,9 @@ typedef HASHBUCKET *HASHSEGMENT;
*
* In a partitioned hash table, each freelist is associated with a specific
* set of hashcodes, as determined by the FREELIST_IDX() macro below.
- * nentries tracks the number of live hashtable entries having those hashcodes
- * (NOT the number of entries in the freelist, as you might expect).
+ * nalloced tracks the number of free hashtable entries initially allocated
+ * for the freelist.
+ * nfree tracks the actual number of free hashtable entries in the freelist.
*
* The coverage of a freelist might be more or less than one partition, so it
* needs its own lock rather than relying on caller locking. Relying on that
@@ -147,13 +148,15 @@ typedef HASHBUCKET *HASHSEGMENT;
* need to "borrow" entries from another freelist; see get_hash_entry().
*
* Using an array of FreeListData instead of separate arrays of mutexes,
- * nentries and freeLists helps to reduce sharing of cache lines between
+ * nfree and freeLists helps to reduce sharing of cache lines between
* different mutexes.
*/
typedef struct
{
slock_t mutex; /* spinlock for this freelist */
- long nentries; /* number of entries in associated buckets */
+ long nfree; /* number of free entries in the list */
+ long nalloced; /* number of entries initially allocated for
+ * the list */
HASHELEMENT *freeList; /* chain of free elements */
} FreeListData;
@@ -170,7 +173,7 @@ struct HASHHDR
/*
* The freelist can become a point of contention in high-concurrency hash
* tables, so we use an array of freelists, each with its own mutex and
- * nentries count, instead of just a single one. Although the freelists
+ * nfree count, instead of just a single one. Although the freelists
* normally operate independently, we will scavenge entries from freelists
* other than a hashcode's default freelist when necessary.
*
@@ -254,6 +257,15 @@ struct HTAB
*/
#define MOD(x,y) ((x) & ((y)-1))
+/*
+ * Struct for reuse element.
+ */
+struct HASHREUSE
+{
+ HTAB *hashp;
+ HASHBUCKET element;
+};
+
#ifdef HASH_STATISTICS
static long hash_accesses,
hash_collisions,
@@ -293,6 +305,12 @@ DynaHashAlloc(Size size)
}
+/*
+ * Support for HASH_REUSE + HASH_ASSIGN
+ */
+static struct HASHREUSE DynaHashReuse = {NULL, NULL};
+
+
/*
* HashCompareFunc for string keys
*
@@ -932,6 +950,10 @@ calc_bucket(HASHHDR *hctl, uint32 hash_val)
* HASH_ENTER: look up key in table, creating entry if not present
* HASH_ENTER_NULL: same, but return NULL if out of memory
* HASH_REMOVE: look up key in table, remove entry if present
+ * HASH_REUSE: same as HASH_REMOVE, but stores removed element in static
+ * variable instead of free list.
+ * HASH_ASSIGN: same as HASH_ENTER, but reuses element stored by HASH_REUSE
+ * if any.
*
* Return value is a pointer to the element found/entered/removed if any,
* or NULL if no match was found. (NB: in the case of the REMOVE action,
@@ -1000,7 +1022,8 @@ hash_search_with_hash_value(HTAB *hashp,
* Can't split if running in partitioned mode, nor if frozen, nor if
* table is the subject of any active hash_seq_search scans.
*/
- if (hctl->freeList[0].nentries > (long) hctl->max_bucket &&
+ if (hctl->freeList[0].nalloced > (long) hctl->max_bucket &&
+ hctl->freeList[0].nfree == 0 &&
!IS_PARTITIONED(hctl) && !hashp->frozen &&
!has_seq_scans(hashp))
(void) expand_table(hashp);
@@ -1044,6 +1067,10 @@ hash_search_with_hash_value(HTAB *hashp,
if (foundPtr)
*foundPtr = (bool) (currBucket != NULL);
+ /* If HASH_REUSE were not called, HASH_ASSIGN falls back to HASH_ENTER */
+ if (action == HASH_ASSIGN && DynaHashReuse.element == NULL)
+ action = HASH_ENTER;
+
/*
* OK, now what?
*/
@@ -1057,20 +1084,17 @@ hash_search_with_hash_value(HTAB *hashp,
case HASH_REMOVE:
if (currBucket != NULL)
{
- /* if partitioned, must lock to touch nentries and freeList */
+ /* if partitioned, must lock to touch nfree and freeList */
if (IS_PARTITIONED(hctl))
SpinLockAcquire(&(hctl->freeList[freelist_idx].mutex));
- /* delete the record from the appropriate nentries counter. */
- Assert(hctl->freeList[freelist_idx].nentries > 0);
- hctl->freeList[freelist_idx].nentries--;
-
/* remove record from hash bucket's chain. */
*prevBucketPtr = currBucket->link;
/* add the record to the appropriate freelist. */
currBucket->link = hctl->freeList[freelist_idx].freeList;
hctl->freeList[freelist_idx].freeList = currBucket;
+ hctl->freeList[freelist_idx].nfree++;
if (IS_PARTITIONED(hctl))
SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
@@ -1116,6 +1140,7 @@ hash_search_with_hash_value(HTAB *hashp,
errmsg("out of memory")));
}
+ link_element:
/* link into hashbucket chain */
*prevBucketPtr = currBucket;
currBucket->link = NULL;
@@ -1132,6 +1157,63 @@ hash_search_with_hash_value(HTAB *hashp,
*/
return (void *) ELEMENTKEY(currBucket);
+
+ case HASH_REUSE:
+ if (currBucket != NULL)
+ {
+ /* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
+ Assert(DynaHashReuse.hashp == NULL);
+ Assert(DynaHashReuse.element == NULL);
+
+ /* remove record from hash bucket's chain. */
+ *prevBucketPtr = currBucket->link;
+
+ /* and store for HASH_ASSIGN */
+ DynaHashReuse.element = currBucket;
+ DynaHashReuse.hashp = hashp;
+
+ /* Caller should call HASH_ASSIGN as the very next step. */
+ return (void *) ELEMENTKEY(currBucket);
+ }
+ return NULL;
+
+ case HASH_ASSIGN:
+ /* check HASH_REUSE were called for same hash table */
+ Assert(DynaHashReuse.hashp == hashp);
+
+ /*
+ * If existing element is found, need to put Reuse element to
+ * original freelist. There is no much difference, which list to
+ * put, since we migrate buckets between buckets.
+ */
+ if (currBucket != NULL)
+ {
+
+ /* if partitioned, must lock to touch nfree and freeList */
+ if (IS_PARTITIONED(hctl))
+ SpinLockAcquire(&(hctl->freeList[freelist_idx].mutex));
+
+ /* add the record to the appropriate freelist. */
+ DynaHashReuse.element->link = hctl->freeList[freelist_idx].freeList;
+ hctl->freeList[freelist_idx].freeList = DynaHashReuse.element;
+ hctl->freeList[freelist_idx].nfree++;
+
+ if (IS_PARTITIONED(hctl))
+ SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
+
+ DynaHashReuse.element = NULL;
+ DynaHashReuse.hashp = NULL;
+
+ return (void *) ELEMENTKEY(currBucket);
+ }
+
+ currBucket = DynaHashReuse.element;
+
+ DynaHashReuse.element = NULL;
+ DynaHashReuse.hashp = NULL;
+
+ /* reuse HASH_ENTER code */
+ goto link_element;
}
elog(ERROR, "unrecognized hash action code: %d", (int) action);
@@ -1301,7 +1383,7 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
for (;;)
{
- /* if partitioned, must lock to touch nentries and freeList */
+ /* if partitioned, must lock to touch nfree and freeList */
if (IS_PARTITIONED(hctl))
SpinLockAcquire(&hctl->freeList[freelist_idx].mutex);
@@ -1347,13 +1429,9 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
if (newElement != NULL)
{
hctl->freeList[borrow_from_idx].freeList = newElement->link;
+ hctl->freeList[borrow_from_idx].nfree--;
SpinLockRelease(&(hctl->freeList[borrow_from_idx].mutex));
- /* careful: count the new element in its proper freelist */
- SpinLockAcquire(&hctl->freeList[freelist_idx].mutex);
- hctl->freeList[freelist_idx].nentries++;
- SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
-
return newElement;
}
@@ -1365,9 +1443,9 @@ get_hash_entry(HTAB *hashp, int freelist_idx)
}
}
- /* remove entry from freelist, bump nentries */
+ /* remove entry from freelist, decrease nfree */
hctl->freeList[freelist_idx].freeList = newElement->link;
- hctl->freeList[freelist_idx].nentries++;
+ hctl->freeList[freelist_idx].nfree--;
if (IS_PARTITIONED(hctl))
SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
@@ -1382,7 +1460,10 @@ long
hash_get_num_entries(HTAB *hashp)
{
int i;
- long sum = hashp->hctl->freeList[0].nentries;
+ long sum = 0;
+
+ sum += hashp->hctl->freeList[0].nalloced;
+ sum -= hashp->hctl->freeList[0].nfree;
/*
* We currently don't bother with acquiring the mutexes; it's only
@@ -1392,7 +1473,10 @@ hash_get_num_entries(HTAB *hashp)
if (IS_PARTITIONED(hashp->hctl))
{
for (i = 1; i < NUM_FREELISTS; i++)
- sum += hashp->hctl->freeList[i].nentries;
+ {
+ sum += hashp->hctl->freeList[i].nalloced;
+ sum -= hashp->hctl->freeList[i].nfree;
+ }
}
return sum;
@@ -1739,6 +1823,8 @@ element_alloc(HTAB *hashp, int nelem, int freelist_idx)
/* freelist could be nonempty if two backends did this concurrently */
firstElement->link = hctl->freeList[freelist_idx].freeList;
hctl->freeList[freelist_idx].freeList = prevElement;
+ hctl->freeList[freelist_idx].nfree += nelem;
+ hctl->freeList[freelist_idx].nalloced += nelem;
if (IS_PARTITIONED(hctl))
SpinLockRelease(&hctl->freeList[freelist_idx].mutex);
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 7c6653311a5..d35ee1b4108 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -328,7 +328,7 @@ extern void InitBufTable(int size);
extern uint32 BufTableHashCode(BufferTag *tagPtr);
extern int BufTableLookup(BufferTag *tagPtr, uint32 hashcode);
extern int BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id);
-extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode);
+extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse);
/* localbuf.c */
extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h
index d7af0239c8c..ba21c5a65a1 100644
--- a/src/include/utils/hsearch.h
+++ b/src/include/utils/hsearch.h
@@ -113,7 +113,9 @@ typedef enum
HASH_FIND,
HASH_ENTER,
HASH_REMOVE,
- HASH_ENTER_NULL
+ HASH_ENTER_NULL,
+ HASH_REUSE,
+ HASH_ASSIGN
} HASHACTION;
/* hash_seq status (should be considered an opaque type by callers) */
--
2.35.1
From b57d77e4d4a7e8b4277d0f88cf09430e1e3163e6 Mon Sep 17 00:00:00 2001
From: Yura Sokolov <[email protected]>
Date: Thu, 3 Mar 2022 01:14:58 +0300
Subject: [PATCH 3/3] reduce memory allocation for non-partitioned dynahash
Non-partitioned hash table doesn't use 32 partitions of HASHHDR->freeList.
Lets allocate just single free list.
---
src/backend/utils/hash/dynahash.c | 37 +++++++++++++++++--------------
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index af4196cf194..cd015abaecf 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -170,18 +170,6 @@ typedef struct
*/
struct HASHHDR
{
- /*
- * The freelist can become a point of contention in high-concurrency hash
- * tables, so we use an array of freelists, each with its own mutex and
- * nfree count, instead of just a single one. Although the freelists
- * normally operate independently, we will scavenge entries from freelists
- * other than a hashcode's default freelist when necessary.
- *
- * If the hash table is not partitioned, only freeList[0] is used and its
- * spinlock is not used at all; callers' locking is assumed sufficient.
- */
- FreeListData freeList[NUM_FREELISTS];
-
/* These fields can change, but not in a partitioned table */
/* Also, dsize can't change in a shared table, even if unpartitioned */
long dsize; /* directory size */
@@ -208,6 +196,18 @@ struct HASHHDR
long accesses;
long collisions;
#endif
+
+ /*
+ * The freelist can become a point of contention in high-concurrency hash
+ * tables, so we use an array of freelists, each with its own mutex and
+ * nfree count, instead of just a single one. Although the freelists
+ * normally operate independently, we will scavenge entries from freelists
+ * other than a hashcode's default freelist when necessary.
+ *
+ * If the hash table is not partitioned, only freeList[0] is used and its
+ * spinlock is not used at all; callers' locking is assumed sufficient.
+ */
+ FreeListData freeList[NUM_FREELISTS];
};
#define IS_PARTITIONED(hctl) ((hctl)->num_partitions != 0)
@@ -281,7 +281,7 @@ static bool element_alloc(HTAB *hashp, int nelem, int freelist_idx);
static bool dir_realloc(HTAB *hashp);
static bool expand_table(HTAB *hashp);
static HASHBUCKET get_hash_entry(HTAB *hashp, int freelist_idx);
-static void hdefault(HTAB *hashp);
+static void hdefault(HTAB *hashp, bool partitioned);
static int choose_nelem_alloc(Size entrysize);
static bool init_htab(HTAB *hashp, long nelem);
static void hash_corrupted(HTAB *hashp);
@@ -524,7 +524,8 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
if (!hashp->hctl)
{
- hashp->hctl = (HASHHDR *) hashp->alloc(sizeof(HASHHDR));
+ Assert(!(flags & HASH_PARTITION));
+ hashp->hctl = (HASHHDR *) hashp->alloc(offsetof(HASHHDR, freeList[1]));
if (!hashp->hctl)
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -533,7 +534,7 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
hashp->frozen = false;
- hdefault(hashp);
+ hdefault(hashp, (flags & HASH_PARTITION) != 0);
hctl = hashp->hctl;
@@ -641,11 +642,13 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
* Set default HASHHDR parameters.
*/
static void
-hdefault(HTAB *hashp)
+hdefault(HTAB *hashp, bool partition)
{
HASHHDR *hctl = hashp->hctl;
- MemSet(hctl, 0, sizeof(HASHHDR));
+ MemSet(hctl, 0, partition ?
+ sizeof(HASHHDR) :
+ offsetof(HASHHDR, freeList[1]));
hctl->dsize = DEF_DIRSIZE;
hctl->nsegs = 0;
--
2.35.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-03-11 06:30 ` Kyotaro Horiguchi <[email protected]>
2022-03-11 06:49 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-11 08:30 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-11 06:30 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 03 Mar 2022 01:35:57 +0300, Yura Sokolov <[email protected]> wrote in
> В Вт, 01/03/2022 в 10:24 +0300, Yura Sokolov пишет:
> > Ok, here is v4.
>
> And here is v5.
>
> First, there was compilation error in Assert in dynahash.c .
> Excuse me for not checking before sending previous version.
>
> Second, I add third commit that reduces HASHHDR allocation
> size for non-partitioned dynahash:
> - moved freeList to last position
> - alloc and memset offset(HASHHDR, freeList[1]) for
> non-partitioned hash tables.
> I didn't benchmarked it, but I will be surprised if it
> matters much in performance sence.
>
> Third, I put all three commits into single file to not
> confuse commitfest application.
Thanks! I looked into dynahash part.
struct HASHHDR
{
- /*
- * The freelist can become a point of contention in high-concurrency hash
Why did you move around the freeList?
- long nentries; /* number of entries in associated buckets */
+ long nfree; /* number of free entries in the list */
+ long nalloced; /* number of entries initially allocated for
Why do we need nfree? HASH_ASSING should do the same thing with
HASH_REMOVE. Maybe the reason is the code tries to put the detached
bucket to different free list, but we can just remember the
freelist_idx for the detached bucket as we do for hashp. I think that
should largely reduce the footprint of this patch.
-static void hdefault(HTAB *hashp);
+static void hdefault(HTAB *hashp, bool partitioned);
That optimization may work even a bit, but it is not irrelevant to
this patch?
+ case HASH_REUSE:
+ if (currBucket != NULL)
+ {
+ /* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
+ Assert(DynaHashReuse.hashp == NULL);
+ Assert(DynaHashReuse.element == NULL);
I think all cases in the switch(action) other than HASH_ASSIGN needs
this assertion and no need for checking both, maybe only for element
would be enough.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
@ 2022-03-11 06:49 ` Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-11 06:49 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 11 Mar 2022 15:30:30 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> Thanks! I looked into dynahash part.
>
> struct HASHHDR
> {
> - /*
> - * The freelist can become a point of contention in high-concurrency hash
>
> Why did you move around the freeList?
>
>
> - long nentries; /* number of entries in associated buckets */
> + long nfree; /* number of free entries in the list */
> + long nalloced; /* number of entries initially allocated for
>
> Why do we need nfree? HASH_ASSING should do the same thing with
> HASH_REMOVE. Maybe the reason is the code tries to put the detached
> bucket to different free list, but we can just remember the
> freelist_idx for the detached bucket as we do for hashp. I think that
> should largely reduce the footprint of this patch.
>
> -static void hdefault(HTAB *hashp);
> +static void hdefault(HTAB *hashp, bool partitioned);
>
> That optimization may work even a bit, but it is not irrelevant to
> this patch?
>
> + case HASH_REUSE:
> + if (currBucket != NULL)
> + {
> + /* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
> + Assert(DynaHashReuse.hashp == NULL);
> + Assert(DynaHashReuse.element == NULL);
>
> I think all cases in the switch(action) other than HASH_ASSIGN needs
> this assertion and no need for checking both, maybe only for element
> would be enough.
While I looked buf_table part, I came up with additional comments.
BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id)
{
hash_search_with_hash_value(SharedBufHash,
HASH_ASSIGN,
...
BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse)
BufTableDelete considers both reuse and !reuse cases but
BufTableInsert doesn't and always does HASH_ASSIGN. That looks
odd. We should use HASH_ENTER here. Thus I think it is more
reasonable that HASH_ENTRY uses the stashed entry if exists and
needed, or returns it to freelist if exists but not needed.
What do you think about this?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
@ 2022-03-11 08:30 ` Yura Sokolov <[email protected]>
2022-03-14 00:39 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Yura Sokolov @ 2022-03-11 08:30 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
В Пт, 11/03/2022 в 15:30 +0900, Kyotaro Horiguchi пишет:
> At Thu, 03 Mar 2022 01:35:57 +0300, Yura Sokolov <[email protected]> wrote in
> > В Вт, 01/03/2022 в 10:24 +0300, Yura Sokolov пишет:
> > > Ok, here is v4.
> >
> > And here is v5.
> >
> > First, there was compilation error in Assert in dynahash.c .
> > Excuse me for not checking before sending previous version.
> >
> > Second, I add third commit that reduces HASHHDR allocation
> > size for non-partitioned dynahash:
> > - moved freeList to last position
> > - alloc and memset offset(HASHHDR, freeList[1]) for
> > non-partitioned hash tables.
> > I didn't benchmarked it, but I will be surprised if it
> > matters much in performance sence.
> >
> > Third, I put all three commits into single file to not
> > confuse commitfest application.
>
> Thanks! I looked into dynahash part.
>
> struct HASHHDR
> {
> - /*
> - * The freelist can become a point of contention in high-concurrency hash
>
> Why did you move around the freeList?
>
>
> - long nentries; /* number of entries in associated buckets */
> + long nfree; /* number of free entries in the list */
> + long nalloced; /* number of entries initially allocated for
>
> Why do we need nfree? HASH_ASSING should do the same thing with
> HASH_REMOVE. Maybe the reason is the code tries to put the detached
> bucket to different free list, but we can just remember the
> freelist_idx for the detached bucket as we do for hashp. I think that
> should largely reduce the footprint of this patch.
If we keep nentries, then we need to fix nentries in both old
"freeList" partition and new one. It is two freeList[partition]->mutex
lock+unlock pairs.
But count of free elements doesn't change, so if we change nentries
to nfree, then no need to fix freeList[partition]->nfree counters,
no need to lock+unlock.
>
> -static void hdefault(HTAB *hashp);
> +static void hdefault(HTAB *hashp, bool partitioned);
>
> That optimization may work even a bit, but it is not irrelevant to
> this patch?
>
> + case HASH_REUSE:
> + if (currBucket != NULL)
> + {
> + /* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
> + Assert(DynaHashReuse.hashp == NULL);
> + Assert(DynaHashReuse.element == NULL);
>
> I think all cases in the switch(action) other than HASH_ASSIGN needs
> this assertion and no need for checking both, maybe only for element
> would be enough.
Agree.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-11 08:30 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-03-14 00:39 ` Kyotaro Horiguchi <[email protected]>
2022-03-14 05:31 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-14 00:39 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 11 Mar 2022 11:30:27 +0300, Yura Sokolov <[email protected]> wrote in
> В Пт, 11/03/2022 в 15:30 +0900, Kyotaro Horiguchi пишет:
> > At Thu, 03 Mar 2022 01:35:57 +0300, Yura Sokolov <[email protected]> wrote in
> > > В Вт, 01/03/2022 в 10:24 +0300, Yura Sokolov пишет:
> > > > Ok, here is v4.
> > >
> > > And here is v5.
> > >
> > > First, there was compilation error in Assert in dynahash.c .
> > > Excuse me for not checking before sending previous version.
> > >
> > > Second, I add third commit that reduces HASHHDR allocation
> > > size for non-partitioned dynahash:
> > > - moved freeList to last position
> > > - alloc and memset offset(HASHHDR, freeList[1]) for
> > > non-partitioned hash tables.
> > > I didn't benchmarked it, but I will be surprised if it
> > > matters much in performance sence.
> > >
> > > Third, I put all three commits into single file to not
> > > confuse commitfest application.
> >
> > Thanks! I looked into dynahash part.
> >
> > struct HASHHDR
> > {
> > - /*
> > - * The freelist can become a point of contention in high-concurrency hash
> >
> > Why did you move around the freeList?
> >
> >
> > - long nentries; /* number of entries in associated buckets */
> > + long nfree; /* number of free entries in the list */
> > + long nalloced; /* number of entries initially allocated for
> >
> > Why do we need nfree? HASH_ASSING should do the same thing with
> > HASH_REMOVE. Maybe the reason is the code tries to put the detached
> > bucket to different free list, but we can just remember the
> > freelist_idx for the detached bucket as we do for hashp. I think that
> > should largely reduce the footprint of this patch.
>
> If we keep nentries, then we need to fix nentries in both old
> "freeList" partition and new one. It is two freeList[partition]->mutex
> lock+unlock pairs.
>
> But count of free elements doesn't change, so if we change nentries
> to nfree, then no need to fix freeList[partition]->nfree counters,
> no need to lock+unlock.
Ah, okay. I missed that bucket reuse chages key in most cases.
But still I don't think its good to move entries around partition
freelists for another reason. I'm afraid that the freelists get into
imbalanced state. get_hash_entry prefers main shmem allocation than
other freelist so that could lead to freelist bloat, or worse
contension than the traditinal way involving more than two partitions.
I'll examine the possibility to resolve this...
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-11 08:30 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-14 00:39 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
@ 2022-03-14 05:31 ` Kyotaro Horiguchi <[email protected]>
2022-03-14 06:15 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-14 05:31 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 14 Mar 2022 09:39:48 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> I'll examine the possibility to resolve this...
The existence of nfree and nalloc made me confused and I found the
reason.
In the case where a parittion collects many REUSE-ASSIGN-REMOVEed
elemetns from other paritiotns, nfree gets larger than nalloced. This
is a strange point of the two counters. nalloced is only referred to
as (sum(nalloced[])). So we don't need nalloced per-partition basis
and the formula to calculate the number of used elements would be as
follows.
sum(nalloced - nfree)
= <total_nalloced> - sum(nfree)
We rarely create fresh elements in shared hashes so I don't think
there's additional contention on the <total_nalloced> even if it were
a global atomic.
So, the remaining issue is the possible imbalancement among
partitions. On second thought, by the current way, if there's a bad
deviation in partition-usage, a heavily hit partition finally collects
elements via get_hash_entry(). By the patch's way, similar thing
happens via the REUSE-ASSIGN-REMOVE sequence. But buffers once used
for something won't be freed until buffer invalidation. But bulk
buffer invalidation won't deviatedly distribute freed buffers among
partitions. So I conclude for now that is a non-issue.
So my opinion on the counters is:
I'd like to ask you to remove nalloced from partitions then add a
global atomic for the same use?
No need to do something for the possible deviation issue.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-11 08:30 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-14 00:39 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-14 05:31 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
@ 2022-03-14 06:15 ` Yura Sokolov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Yura Sokolov @ 2022-03-14 06:15 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
В Пн, 14/03/2022 в 14:31 +0900, Kyotaro Horiguchi пишет:
> At Mon, 14 Mar 2022 09:39:48 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > I'll examine the possibility to resolve this...
>
> The existence of nfree and nalloc made me confused and I found the
> reason.
>
> In the case where a parittion collects many REUSE-ASSIGN-REMOVEed
> elemetns from other paritiotns, nfree gets larger than nalloced. This
> is a strange point of the two counters. nalloced is only referred to
> as (sum(nalloced[])). So we don't need nalloced per-partition basis
> and the formula to calculate the number of used elements would be as
> follows.
>
> sum(nalloced - nfree)
> = <total_nalloced> - sum(nfree)
>
> We rarely create fresh elements in shared hashes so I don't think
> there's additional contention on the <total_nalloced> even if it were
> a global atomic.
>
> So, the remaining issue is the possible imbalancement among
> partitions. On second thought, by the current way, if there's a bad
> deviation in partition-usage, a heavily hit partition finally collects
> elements via get_hash_entry(). By the patch's way, similar thing
> happens via the REUSE-ASSIGN-REMOVE sequence. But buffers once used
> for something won't be freed until buffer invalidation. But bulk
> buffer invalidation won't deviatedly distribute freed buffers among
> partitions. So I conclude for now that is a non-issue.
>
> So my opinion on the counters is:
>
> I'd like to ask you to remove nalloced from partitions then add a
> global atomic for the same use?
I really believe it should be global. I made it per-partition to
not overcomplicate first versions. Glad you tell it.
I thought to protect it with freeList[0].mutex, but probably atomic
is better idea here. But which atomic to chose: uint64 or uint32?
Based on sizeof(long)?
Ok, I'll do in next version.
Whole get_hash_entry look strange.
Doesn't it better to cycle through partitions and only then go to
get_hash_entry?
May be there should be bitmap for non-empty free lists? 32bit for
32 partitions. But wouldn't bitmap became contention point itself?
> No need to do something for the possible deviation issue.
-------
regards
Yura Sokolov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-02-25 08:04 ` Andres Freund <[email protected]>
2022-02-25 09:14 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-25 09:51 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
1 sibling, 2 replies; 20+ messages in thread
From: Andres Freund @ 2022-02-25 08:04 UTC (permalink / raw)
To: Yura Sokolov <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]
Hi,
On 2022-02-21 11:06:49 +0300, Yura Sokolov wrote:
> From 04b07d0627ec65ba3327dc8338d59dbd15c405d8 Mon Sep 17 00:00:00 2001
> From: Yura Sokolov <[email protected]>
> Date: Mon, 21 Feb 2022 08:49:03 +0300
> Subject: [PATCH v3] [PGPRO-5616] bufmgr: do not acquire two partition locks.
>
> Acquiring two partition locks leads to complex dependency chain that hurts
> at high concurrency level.
>
> There is no need to hold both lock simultaneously. Buffer is pinned so
> other processes could not select it for eviction. If tag is cleared and
> buffer removed from old partition other processes will not find it.
> Therefore it is safe to release old partition lock before acquiring
> new partition lock.
Yes, the current design is pretty nonsensical. It leads to really absurd stuff
like holding the relation extension lock while we write out old buffer
contents etc.
> + * We have pinned buffer and we are single pinner at the moment so there
> + * is no other pinners.
Seems redundant.
> We hold buffer header lock and exclusive partition
> + * lock if tag is valid. Given these statements it is safe to clear tag
> + * since no other process can inspect it to the moment.
> + */
Could we share code with InvalidateBuffer here? It's not quite the same code,
but nearly the same.
> + * The usage_count starts out at 1 so that the buffer can survive one
> + * clock-sweep pass.
> + *
> + * We use direct atomic OR instead of Lock+Unlock since no other backend
> + * could be interested in the buffer. But StrategyGetBuffer,
> + * Flush*Buffers, Drop*Buffers are scanning all buffers and locks them to
> + * compare tag, and UnlockBufHdr does raw write to state. So we have to
> + * spin if we found buffer locked.
So basically the first half of of the paragraph is wrong, because no, we
can't?
> + * Note that we write tag unlocked. It is also safe since there is always
> + * check for BM_VALID when tag is compared.
> */
> buf->tag = newTag;
> - buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
> - BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
> - BUF_USAGECOUNT_MASK);
> if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
> - buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> + new_bits = BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> else
> - buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> -
> - UnlockBufHdr(buf, buf_state);
> + new_bits = BM_TAG_VALID | BUF_USAGECOUNT_ONE;
>
> - if (oldPartitionLock != NULL)
> + buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
> + while (unlikely(buf_state & BM_LOCKED))
I don't think it's safe to atomic in arbitrary bits. If somebody else has
locked the buffer header in this moment, it'll lead to completely bogus
results, because unlocking overwrites concurrently written contents (which
there shouldn't be any, but here there are)...
And or'ing contents in also doesn't make sense because we it doesn't work to
actually unset any contents?
Why don't you just use LockBufHdr/UnlockBufHdr?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 08:04 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
@ 2022-02-25 09:14 ` Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 20+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 09:14 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 25 Feb 2022 00:04:55 -0800, Andres Freund <[email protected]> wrote in
> Why don't you just use LockBufHdr/UnlockBufHdr?
FWIW, v2 looked fine to me in regards to this point.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 08:04 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
@ 2022-02-25 09:51 ` Yura Sokolov <[email protected]>
2022-02-25 17:01 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Yura Sokolov @ 2022-02-25 09:51 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]
Hello, Andres
В Пт, 25/02/2022 в 00:04 -0800, Andres Freund пишет:
> Hi,
>
> On 2022-02-21 11:06:49 +0300, Yura Sokolov wrote:
> > From 04b07d0627ec65ba3327dc8338d59dbd15c405d8 Mon Sep 17 00:00:00 2001
> > From: Yura Sokolov <[email protected]>
> > Date: Mon, 21 Feb 2022 08:49:03 +0300
> > Subject: [PATCH v3] [PGPRO-5616] bufmgr: do not acquire two partition locks.
> >
> > Acquiring two partition locks leads to complex dependency chain that hurts
> > at high concurrency level.
> >
> > There is no need to hold both lock simultaneously. Buffer is pinned so
> > other processes could not select it for eviction. If tag is cleared and
> > buffer removed from old partition other processes will not find it.
> > Therefore it is safe to release old partition lock before acquiring
> > new partition lock.
>
> Yes, the current design is pretty nonsensical. It leads to really absurd stuff
> like holding the relation extension lock while we write out old buffer
> contents etc.
>
>
>
> > + * We have pinned buffer and we are single pinner at the moment so there
> > + * is no other pinners.
>
> Seems redundant.
>
>
> > We hold buffer header lock and exclusive partition
> > + * lock if tag is valid. Given these statements it is safe to clear tag
> > + * since no other process can inspect it to the moment.
> > + */
>
> Could we share code with InvalidateBuffer here? It's not quite the same code,
> but nearly the same.
>
>
> > + * The usage_count starts out at 1 so that the buffer can survive one
> > + * clock-sweep pass.
> > + *
> > + * We use direct atomic OR instead of Lock+Unlock since no other backend
> > + * could be interested in the buffer. But StrategyGetBuffer,
> > + * Flush*Buffers, Drop*Buffers are scanning all buffers and locks them to
> > + * compare tag, and UnlockBufHdr does raw write to state. So we have to
> > + * spin if we found buffer locked.
>
> So basically the first half of of the paragraph is wrong, because no, we
> can't?
Logically, there are no backends that could be interesting in the buffer.
Physically they do LockBufHdr/UnlockBufHdr just to check they are not interesting.
> > + * Note that we write tag unlocked. It is also safe since there is always
> > + * check for BM_VALID when tag is compared.
>
>
> > */
> > buf->tag = newTag;
> > - buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
> > - BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
> > - BUF_USAGECOUNT_MASK);
> > if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
> > - buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> > + new_bits = BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> > else
> > - buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> > -
> > - UnlockBufHdr(buf, buf_state);
> > + new_bits = BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> >
> > - if (oldPartitionLock != NULL)
> > + buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
> > + while (unlikely(buf_state & BM_LOCKED))
>
> I don't think it's safe to atomic in arbitrary bits. If somebody else has
> locked the buffer header in this moment, it'll lead to completely bogus
> results, because unlocking overwrites concurrently written contents (which
> there shouldn't be any, but here there are)...
That is why there is safety loop in the case buf->state were locked just
after first optimistic atomic_fetch_or. 99.999% times this loop will not
have a job. But in case other backend did lock buf->state, loop waits
until it releases lock and retry atomic_fetch_or.
> And or'ing contents in also doesn't make sense because we it doesn't work to
> actually unset any contents?
Sorry, I didn't understand sentence :((
> Why don't you just use LockBufHdr/UnlockBufHdr?
This pair makes two atomic writes to memory. Two writes are heavier than
one write in this version (if optimistic case succeed).
But I thought to use Lock+UnlockBuhHdr instead of safety loop:
buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
if (unlikely(buf_state & BM_LOCKED))
{
buf_state = LockBufHdr(&buf->state);
UnlockBufHdr(&buf->state, buf_state | new_bits);
}
I agree this way code is cleaner. Will do in next version.
-----
regards,
Yura Sokolov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 08:04 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
2022-02-25 09:51 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
@ 2022-02-25 17:01 ` Andres Freund <[email protected]>
2022-02-28 06:01 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2022-02-25 17:01 UTC (permalink / raw)
To: Yura Sokolov <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]
Hi,
On 2022-02-25 12:51:22 +0300, Yura Sokolov wrote:
> > > + * The usage_count starts out at 1 so that the buffer can survive one
> > > + * clock-sweep pass.
> > > + *
> > > + * We use direct atomic OR instead of Lock+Unlock since no other backend
> > > + * could be interested in the buffer. But StrategyGetBuffer,
> > > + * Flush*Buffers, Drop*Buffers are scanning all buffers and locks them to
> > > + * compare tag, and UnlockBufHdr does raw write to state. So we have to
> > > + * spin if we found buffer locked.
> >
> > So basically the first half of of the paragraph is wrong, because no, we
> > can't?
>
> Logically, there are no backends that could be interesting in the buffer.
> Physically they do LockBufHdr/UnlockBufHdr just to check they are not interesting.
Yea, but that's still being interested in the buffer...
> > > + * Note that we write tag unlocked. It is also safe since there is always
> > > + * check for BM_VALID when tag is compared.
> >
> >
> > > */
> > > buf->tag = newTag;
> > > - buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
> > > - BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
> > > - BUF_USAGECOUNT_MASK);
> > > if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
> > > - buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> > > + new_bits = BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> > > else
> > > - buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> > > -
> > > - UnlockBufHdr(buf, buf_state);
> > > + new_bits = BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> > >
> > > - if (oldPartitionLock != NULL)
> > > + buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
> > > + while (unlikely(buf_state & BM_LOCKED))
> >
> > I don't think it's safe to atomic in arbitrary bits. If somebody else has
> > locked the buffer header in this moment, it'll lead to completely bogus
> > results, because unlocking overwrites concurrently written contents (which
> > there shouldn't be any, but here there are)...
>
> That is why there is safety loop in the case buf->state were locked just
> after first optimistic atomic_fetch_or. 99.999% times this loop will not
> have a job. But in case other backend did lock buf->state, loop waits
> until it releases lock and retry atomic_fetch_or.
> > And or'ing contents in also doesn't make sense because we it doesn't work to
> > actually unset any contents?
>
> Sorry, I didn't understand sentence :((
You're OR'ing multiple bits into buf->state. LockBufHdr() only ORs in
BM_LOCKED. ORing BM_LOCKED is fine:
Either the buffer is not already locked, in which case it just sets the
BM_LOCKED bit, acquiring the lock. Or it doesn't change anything, because
BM_LOCKED already was set.
But OR'ing in multiple bits is *not* fine, because it'll actually change the
contents of ->state while the buffer header is locked.
> > Why don't you just use LockBufHdr/UnlockBufHdr?
>
> This pair makes two atomic writes to memory. Two writes are heavier than
> one write in this version (if optimistic case succeed).
UnlockBufHdr doesn't use a locked atomic op. It uses a write barrier and an
unlocked write.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BufferAlloc: don't take two simultaneous locks
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 08:04 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
2022-02-25 09:51 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 17:01 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
@ 2022-02-28 06:01 ` Yura Sokolov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Yura Sokolov @ 2022-02-28 06:01 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]
В Пт, 25/02/2022 в 09:01 -0800, Andres Freund пишет:
> Hi,
>
> On 2022-02-25 12:51:22 +0300, Yura Sokolov wrote:
> > > > + * The usage_count starts out at 1 so that the buffer can survive one
> > > > + * clock-sweep pass.
> > > > + *
> > > > + * We use direct atomic OR instead of Lock+Unlock since no other backend
> > > > + * could be interested in the buffer. But StrategyGetBuffer,
> > > > + * Flush*Buffers, Drop*Buffers are scanning all buffers and locks them to
> > > > + * compare tag, and UnlockBufHdr does raw write to state. So we have to
> > > > + * spin if we found buffer locked.
> > >
> > > So basically the first half of of the paragraph is wrong, because no, we
> > > can't?
> >
> > Logically, there are no backends that could be interesting in the buffer.
> > Physically they do LockBufHdr/UnlockBufHdr just to check they are not interesting.
>
> Yea, but that's still being interested in the buffer...
>
>
> > > > + * Note that we write tag unlocked. It is also safe since there is always
> > > > + * check for BM_VALID when tag is compared.
> > > > */
> > > > buf->tag = newTag;
> > > > - buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
> > > > - BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
> > > > - BUF_USAGECOUNT_MASK);
> > > > if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
> > > > - buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> > > > + new_bits = BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
> > > > else
> > > > - buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> > > > -
> > > > - UnlockBufHdr(buf, buf_state);
> > > > + new_bits = BM_TAG_VALID | BUF_USAGECOUNT_ONE;
> > > >
> > > > - if (oldPartitionLock != NULL)
> > > > + buf_state = pg_atomic_fetch_or_u32(&buf->state, new_bits);
> > > > + while (unlikely(buf_state & BM_LOCKED))
> > >
> > > I don't think it's safe to atomic in arbitrary bits. If somebody else has
> > > locked the buffer header in this moment, it'll lead to completely bogus
> > > results, because unlocking overwrites concurrently written contents (which
> > > there shouldn't be any, but here there are)...
> >
> > That is why there is safety loop in the case buf->state were locked just
> > after first optimistic atomic_fetch_or. 99.999% times this loop will not
> > have a job. But in case other backend did lock buf->state, loop waits
> > until it releases lock and retry atomic_fetch_or.
> > > And or'ing contents in also doesn't make sense because we it doesn't work to
> > > actually unset any contents?
> >
> > Sorry, I didn't understand sentence :((
>
> You're OR'ing multiple bits into buf->state. LockBufHdr() only ORs in
> BM_LOCKED. ORing BM_LOCKED is fine:
> Either the buffer is not already locked, in which case it just sets the
> BM_LOCKED bit, acquiring the lock. Or it doesn't change anything, because
> BM_LOCKED already was set.
>
> But OR'ing in multiple bits is *not* fine, because it'll actually change the
> contents of ->state while the buffer header is locked.
First, both states are valid: before atomic_or and after.
Second, there are no checks for buffer->state while buffer header is locked.
All LockBufHdr users uses result of LockBufHdr. (I just checked that).
> > > Why don't you just use LockBufHdr/UnlockBufHdr?
> >
> > This pair makes two atomic writes to memory. Two writes are heavier than
> > one write in this version (if optimistic case succeed).
>
> UnlockBufHdr doesn't use a locked atomic op. It uses a write barrier and an
> unlocked write.
Write barrier is not free on any platform.
Well, while I don't see problem with modifying buffer->state, there is problem
with modifying buffer->tag: I missed Drop*Buffers doesn't check BM_TAG_VALID
flag. Therefore either I had to add this check to those places, or return to
LockBufHdr+UnlockBufHdr pair.
For patch simplicity I'll return Lock+UnlockBufHdr pair. But it has measurable
impact on low connection numbers on many-sockets.
>
> Greetings,
>
> Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH v2] Use open file description locks for data directory lockfile
@ 2025-12-18 17:21 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Dmitrii Dolgov @ 2025-12-18 17:21 UTC (permalink / raw)
When starting up, postmaster checks for an existing data directory lockfile. If
this file contains current process PID, it's assumed to be stale. Turns out
there is another possibility: we might be running in a PID namespace, and there
is another postgres running inside another PID namespace using the same data
directory. The result is that we don't see another process due to namespace
isolation and start concurrently with the other.
To prevent such situations, at startup use fcntl to get an exclusive open file
description lock for data directory lockfile. Since such locks are associated
with open file descriptors, meaning they're not affected by PID namespace
isolation. It's a "best effort" locking, intended to work with already existing
mechanism, not replace it.
This approach was discussed multiple times in the past, and usually was
rejected as the main work horse for the data directory lockfile due to:
* Portability issues. Open file description lock was a non-POSIX extension in
Linux and similar flock is from BSD standard. But looks like everybody agrees
that such locks make more sense than a typical advisory locks, and
F_OFD_SETLK made its way into POSIX.1 2024 [1].
* Issues with NFS. The current state of things here looks like this:
- NFSv3 doesn't implement open file description locks, they're converted to
advisory locks instead. Advisory locks are subject to namespace isolation,
meaning that processes in different PID namespaces will not see each other
advisory lock, and it's still possible to run multiple postgres
instances on the same data directory.
- NFSv4 uses a lease system for locking, I haven't found any mention of
conversion to advisory locks neither in the man page nor in RFC [2].
To summarize, the approach is now considered POSIX and should fix the described
problem everywhere, except NFSv3.
[1]: https://pubs.opengroup.org/onlinepubs/9799919799/functions/fcntl.html
[2]: https://www.rfc-editor.org/rfc/rfc7530
---
configure | 14 ++++
configure.ac | 3 +
meson.build | 1 +
src/backend/utils/init/miscinit.c | 107 +++++++++++++++++++++++++-----
src/include/pg_config.h.in | 4 ++
5 files changed, 111 insertions(+), 18 deletions(-)
diff --git a/configure b/configure
index fb6a4914b06..1da073f0aaa 100755
--- a/configure
+++ b/configure
@@ -16177,6 +16177,20 @@ cat >>confdefs.h <<_ACEOF
_ACEOF
+# Linux open file descriptor locks
+ac_fn_c_check_decl "$LINENO" "F_OFD_SETLK" "ac_cv_have_decl_F_OFD_SETLK" "#include <fcntl.h>
+"
+if test "x$ac_cv_have_decl_F_OFD_SETLK" = xyes; then :
+ ac_have_decl=1
+else
+ ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_F_OFD_SETLK $ac_have_decl
+_ACEOF
+
+
ac_fn_c_check_func "$LINENO" "explicit_bzero" "ac_cv_func_explicit_bzero"
if test "x$ac_cv_func_explicit_bzero" = xyes; then :
$as_echo "#define HAVE_EXPLICIT_BZERO 1" >>confdefs.h
diff --git a/configure.ac b/configure.ac
index d3febfe58f1..075194a8af8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1844,6 +1844,9 @@ AC_CHECK_DECLS([memset_s], [], [], [#define __STDC_WANT_LIB_EXT1__ 1
# This is probably only present on macOS, but may as well check always
AC_CHECK_DECLS(F_FULLFSYNC, [], [], [#include <fcntl.h>])
+# Linux open file descriptor locks
+AC_CHECK_DECLS([F_OFD_SETLK], [], [], [#include <fcntl.h>])
+
AC_REPLACE_FUNCS(m4_normalize([
explicit_bzero
getopt
diff --git a/meson.build b/meson.build
index 6d304f32fb0..d346aefe9b6 100644
--- a/meson.build
+++ b/meson.build
@@ -2694,6 +2694,7 @@ decl_checks = [
['strlcpy', 'string.h'],
['strsep', 'string.h'],
['timingsafe_bcmp', 'string.h'],
+ ['F_OFD_SETLK', 'fcntl.h'],
]
# Need to check for function declarations for these functions, because
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 563f20374ff..b41a5f9dcca 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -68,6 +68,11 @@ static List *lock_files = NIL;
static Latch LocalLatchData;
+#if HAVE_DECL_F_OFD_SETLK
+/* File descriptor for data directory lock file. */
+static int DataDirLockFD;
+#endif
+
/* ----------------------------------------------------------------
* ignoring system indexes support stuff
*
@@ -1117,6 +1122,45 @@ RestoreClientConnectionInfo(char *conninfo)
*-------------------------------------------------------------------------
*/
+/*
+ * Flock the data directory lockfile.
+ *
+ * Lock the data directory lockfile with an open file description lock. If the
+ * lock is already taken, it's a hard stop. It's only a best effort test, and
+ * any other errors are ignored. On succes the file descriptor is duplicated,
+ * to make sure there will be at least one open copy of it to keep the lock.
+ *
+ * filename is used only for reporting purposes.
+ */
+static void
+FlockDataDirLockFile(int fd, const char *filename)
+{
+
+#if HAVE_DECL_F_OFD_SETLK
+ struct flock lock;
+
+ lock.l_type = F_WRLCK;
+ lock.l_whence = SEEK_SET;
+ lock.l_start = 0;
+ lock.l_len = 0;
+ lock.l_pid = 0;
+
+ if (fcntl(fd, F_OFD_SETLK, &lock) == -1)
+ {
+ if (errno == EAGAIN)
+ ereport(FATAL,
+ (errcode(ERRCODE_LOCK_FILE_EXISTS),
+ errmsg("cannot lock the lock file \"%s\"", filename),
+ errhint("Another server is starting.")));
+ else
+ elog(WARNING, "Failed locking file \"%s\", %m", filename);
+ }
+ else
+ DataDirLockFD = dup(fd);
+#endif
+
+}
+
/*
* proc_exit callback to remove lockfiles.
*/
@@ -1125,6 +1169,11 @@ UnlinkLockFiles(int status, Datum arg)
{
ListCell *l;
+#if HAVE_DECL_F_OFD_SETLK
+ /* Close the file descriptor, which keeps the open file description lock */
+ close(DataDirLockFD);
+#endif
+
foreach(l, lock_files)
{
char *curfile = (char *) lfirst(l);
@@ -1171,22 +1220,32 @@ CreateLockFile(const char *filename, bool amPostmaster,
const char *envvar;
/*
- * If the PID in the lockfile is our own PID or our parent's or
- * grandparent's PID, then the file must be stale (probably left over from
- * a previous system boot cycle). We need to check this because of the
- * likelihood that a reboot will assign exactly the same PID as we had in
- * the previous reboot, or one that's only one or two counts larger and
- * hence the lockfile's PID now refers to an ancestor shell process. We
- * allow pg_ctl to pass down its parent shell PID (our grandparent PID)
- * via the environment variable PG_GRANDPARENT_PID; this is so that
- * launching the postmaster via pg_ctl can be just as reliable as
- * launching it directly. There is no provision for detecting
- * further-removed ancestor processes, but if the init script is written
- * carefully then all but the immediate parent shell will be root-owned
- * processes and so the kill test will fail with EPERM. Note that we
- * cannot get a false negative this way, because an existing postmaster
- * would surely never launch a competing postmaster or pg_ctl process
- * directly.
+ * If we find an already existing lockfile containing our own PID,
+ * there are few options:
+ *
+ * - There is another process, that we don't see due to PID namespace
+ * isolation, which is already running in this data directory.
+ *
+ * To prevent two concurrent processes working with the same data
+ * directory, we first try to lock the lockfile exclusively.
+ *
+ * - The file must be stale, probably left over from a previous system boot
+ * cycle. The same if the lockfile contains our parent's or grandparent's
+ * PID.
+ *
+ * We need to check this because of the likelihood that a reboot will
+ * assign exactly the same PID as we had in the previous reboot, or one
+ * that's only one or two counts larger and hence the lockfile's PID now
+ * refers to an ancestor shell process. We allow pg_ctl to pass down its
+ * parent shell PID (our grandparent PID) via the environment variable
+ * PG_GRANDPARENT_PID; this is so that launching the postmaster via
+ * pg_ctl can be just as reliable as launching it directly. There is no
+ * provision for detecting further-removed ancestor processes, but if the
+ * init script is written carefully then all but the immediate parent
+ * shell will be root-owned processes and so the kill test will fail with
+ * EPERM. Note that we cannot get a false negative this way, because an
+ * existing postmaster would surely never launch a competing postmaster
+ * or pg_ctl process directly.
*/
my_pid = getpid();
@@ -1222,7 +1281,11 @@ CreateLockFile(const char *filename, bool amPostmaster,
*/
fd = open(filename, O_RDWR | O_CREAT | O_EXCL, pg_file_create_mode);
if (fd >= 0)
- break; /* Success; exit the retry loop */
+ {
+ /* Success; lock and exit the retry loop */
+ FlockDataDirLockFile(fd, filename);
+ break;
+ }
/*
* Couldn't create the pid file. Probably it already exists.
@@ -1236,8 +1299,12 @@ CreateLockFile(const char *filename, bool amPostmaster,
/*
* Read the file to get the old owner's PID. Note race condition
* here: file might have been deleted since we tried to create it.
+ *
+ * We're going to use the same fd for flock, and want to create a write
+ * lock for the latter one. Since both fd and the lock have to be of
+ * the same type, open the file for read and write.
*/
- fd = open(filename, O_RDONLY, pg_file_create_mode);
+ fd = open(filename, O_RDWR, pg_file_create_mode);
if (fd < 0)
{
if (errno == ENOENT)
@@ -1247,6 +1314,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
errmsg("could not open lock file \"%s\": %m",
filename)));
}
+
+ /* Try to lock the file. We stop here, if it's already locked. */
+ FlockDataDirLockFile(fd, filename);
+
pgstat_report_wait_start(WAIT_EVENT_LOCK_FILE_CREATE_READ);
if ((len = read(fd, buffer, sizeof(buffer) - 1)) < 0)
ereport(FATAL,
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 339268dc8ef..e7b8e023829 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -80,6 +80,10 @@
don't. */
#undef HAVE_DECL_F_FULLFSYNC
+/* Define to 1 if you have the declaration of `F_OFD_SETLK', and to 0 if you
+ don't. */
+#undef HAVE_DECL_F_OFD_SETLK
+
/* Define to 1 if you have the declaration of `memset_s', and to 0 if you
don't. */
#undef HAVE_DECL_MEMSET_S
base-commit: 6831cd9e3b082d7b830c3196742dd49e3540c49b
--
2.49.0
--7u2oub7w3nl66bkx--
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2025-12-18 17:21 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-02-20 10:38 [PATCH v1] Hide other user's pg_stat_ssl rows Peter Eisentraut <[email protected]>
2022-02-17 05:16 Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-21 08:06 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 04:35 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-02-25 09:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 09:38 ` Re: BufferAlloc: don't take two simultaneous locks Simon Riggs <[email protected]>
2022-03-01 07:24 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-02 22:35 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-11 06:30 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-11 06:49 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-11 08:30 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-03-14 00:39 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-14 05:31 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-03-14 06:15 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 08:04 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
2022-02-25 09:14 ` Re: BufferAlloc: don't take two simultaneous locks Kyotaro Horiguchi <[email protected]>
2022-02-25 09:51 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2022-02-25 17:01 ` Re: BufferAlloc: don't take two simultaneous locks Andres Freund <[email protected]>
2022-02-28 06:01 ` Re: BufferAlloc: don't take two simultaneous locks Yura Sokolov <[email protected]>
2025-12-18 17:21 [PATCH v2] Use open file description locks for data directory lockfile Dmitrii Dolgov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox