Thanks for the new version. At Tue, 15 Mar 2022 08:07:39 +0300, Yura Sokolov <y.soko...@postgrespro.ru> wrote in > В Пн, 14/03/2022 в 14:57 +0300, Yura Sokolov пишет: > > В Пн, 14/03/2022 в 17:12 +0900, Kyotaro Horiguchi пишет: > > > At Mon, 14 Mar 2022 09:15:11 +0300, Yura Sokolov > > > <y.soko...@postgrespro.ru> wrote in > > > > В Пн, 14/03/2022 в 14:31 +0900, Kyotaro Horiguchi пишет: > > > I tried pgbench runs with scale 100 (with 10 threads, 10 clients) on > > > 128kB shared buffers and I saw that get_hash_entry never takes the > > > !element_alloc() path and always allocate a fresh entry, then > > > saturates at 30 new elements allocated at the medium of a 100 seconds > > > run. > > > > > > Then, I tried the same with the patch, and I am surprized to see that > > > the rise of the number of newly allocated elements didn't stop and > > > went up to 511 elements after the 100 seconds run. So I found that my > > > concern was valid. The change in dynahash actually > > > continuously/repeatedly causes lack of free list entries. I'm not > > > sure how much the impact given on performance if we change > > > get_hash_entry to prefer other freelists, though. > > > > Well, it is quite strange SharedBufHash is not allocated as > > HASH_FIXED_SIZE. Could you check what happens with this flag set? > > I'll try as well. > > > > Other way to reduce observed case is to remember freelist_idx for > > reused entry. I didn't believe it matters much since entries migrated > > netherless, but probably due to some hot buffers there are tention to > > crowd particular freelist. > > Well, I did both. Everything looks ok.
Hmm. v8 returns stashed element with original patition index when the element is *not* reused. But what I saw in the previous test runs is the REUSE->ENTER(reuse)(->REMOVE) case. So the new version looks like behaving the same way (or somehow even worse) with the previous version. get_hash_entry continuously suffer lack of freelist entry. (FWIW, attached are the test-output diff for both master and patched) master finally allocated 31 fresh elements for a 100s run. > ALLOCED: 31 ;; freshly allocated v8 finally borrowed 33620 times from another freelist and 0 freshly allocated (ah, this version changes that..) Finally v8 results in: > RETURNED: 50806 ;; returned stashed elements > BORROWED: 33620 ;; borrowed from another freelist > REUSED: 1812664 ;; stashed > ASSIGNED: 1762377 ;; reused >(ALLOCED: 0) ;; freshly allocated It contains a huge degradation by frequent elog's so they cannot be naively relied on, but it should show what is happening sufficiently. > I lost access to Xeon 8354H, so returned to old Xeon X5675. ... > Strange thing: both master and patched version has higher > peak tps at X5676 at medium connections (17 or 27 clients) > than in first october version [1]. But lower tps at higher > connections number (>= 191 clients). > I'll try to bisect on master this unfortunate change. The reversing of the preference order between freshly-allocation and borrow-from-another-freelist might affect. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c index dc439940fa..ac651b98e6 100644 --- a/src/backend/storage/buffer/buf_table.c +++ b/src/backend/storage/buffer/buf_table.c @@ -31,7 +31,7 @@ typedef struct int id; /* Associated buffer ID */ } BufferLookupEnt; -static HTAB *SharedBufHash; +HTAB *SharedBufHash; /* diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 3babde8d70..294516ef01 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -195,6 +195,11 @@ struct HASHHDR long ssize; /* segment size --- must be power of 2 */ int sshift; /* segment shift = log2(ssize) */ int nelem_alloc; /* number of entries to allocate at once */ + int alloc; + int reuse; + int borrow; + int assign; + int ret; #ifdef HASH_STATISTICS @@ -963,6 +968,7 @@ hash_search(HTAB *hashp, foundPtr); } +extern HTAB *SharedBufHash; void * hash_search_with_hash_value(HTAB *hashp, const void *keyPtr, @@ -1354,6 +1360,8 @@ get_hash_entry(HTAB *hashp, int freelist_idx) hctl->freeList[freelist_idx].nentries++; SpinLockRelease(&hctl->freeList[freelist_idx].mutex); + if (hashp == SharedBufHash) + elog(LOG, "BORROWED: %d", ++hctl->borrow); return newElement; } @@ -1363,6 +1371,8 @@ get_hash_entry(HTAB *hashp, int freelist_idx) /* no elements available to borrow either, so out of memory */ return NULL; } + else if (hashp == SharedBufHash) + elog(LOG, "ALLOCED: %d", ++hctl->alloc); } /* remove entry from freelist, bump nentries */
diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c index 55bb491ad0..029bb89f26 100644 --- a/src/backend/storage/buffer/buf_table.c +++ b/src/backend/storage/buffer/buf_table.c @@ -31,7 +31,7 @@ typedef struct int id; /* Associated buffer ID */ } BufferLookupEnt; -static HTAB *SharedBufHash; +HTAB *SharedBufHash; /* diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 50c0e47643..00159714d1 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -199,6 +199,11 @@ struct HASHHDR int nelem_alloc; /* number of entries to allocate at once */ nalloced_t nalloced; /* number of entries allocated */ + int alloc; + int reuse; + int borrow; + int assign; + int ret; #ifdef HASH_STATISTICS /* @@ -1006,6 +1011,7 @@ hash_search(HTAB *hashp, foundPtr); } +extern HTAB *SharedBufHash; void * hash_search_with_hash_value(HTAB *hashp, const void *keyPtr, @@ -1143,6 +1149,8 @@ hash_search_with_hash_value(HTAB *hashp, DynaHashReuse.hashp = hashp; DynaHashReuse.freelist_idx = freelist_idx; + if (hashp == SharedBufHash) + elog(LOG, "REUSED: %d", ++hctl->reuse); /* Caller should call HASH_ASSIGN as the very next step. */ return (void *) ELEMENTKEY(currBucket); } @@ -1160,6 +1168,9 @@ hash_search_with_hash_value(HTAB *hashp, if (likely(DynaHashReuse.element == NULL)) return (void *) ELEMENTKEY(currBucket); + if (hashp == SharedBufHash) + elog(LOG, "RETURNED: %d", ++hctl->ret); + freelist_idx = DynaHashReuse.freelist_idx; /* if partitioned, must lock to touch nfree and freeList */ if (IS_PARTITIONED(hctl)) @@ -1191,6 +1202,13 @@ hash_search_with_hash_value(HTAB *hashp, } else { + if (hashp == SharedBufHash) + { + hctl->assign++; + elog(LOG, "ASSIGNED: %d (%d)", + hctl->assign, hctl->reuse - hctl->assign); + } + currBucket = DynaHashReuse.element; DynaHashReuse.element = NULL; DynaHashReuse.hashp = NULL; @@ -1448,6 +1466,8 @@ get_hash_entry(HTAB *hashp, int freelist_idx) hctl->freeList[borrow_from_idx].nfree--; SpinLockRelease(&(hctl->freeList[borrow_from_idx].mutex)); + if (hashp == SharedBufHash) + elog(LOG, "BORROWED: %d", ++hctl->borrow); return newElement; } @@ -1457,6 +1477,10 @@ get_hash_entry(HTAB *hashp, int freelist_idx) /* no elements available to borrow either, so out of memory */ return NULL; } + else if (hashp == SharedBufHash) + elog(LOG, "ALLOCED: %d", ++hctl->alloc); + + } /* remove entry from freelist, decrease nfree */