Re: BufferAlloc: don't take two simultaneous locks

В Пн, 14/03/2022 в 17:12 +0900, Kyotaro Horiguchi пишет:
> At Mon, 14 Mar 2022 09:15:11 +0300, Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru> wrote in
> > В Пн, 14/03/2022 в 14:31 +0900, Kyotaro Horiguchi пишет:
> > > 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.
>
> Current nentries is a long (= int64 on CentOS). And uint32 can support
> roughly 2^32 * 8192 = 32TB shared buffers, which doesn't seem safe
> enough. So it would be uint64.
>
> > 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?
>
> The code puts significance on avoiding contention caused by visiting
> freelists of other partitions. And perhaps thinks that freelist
> shortage rarely happen.
>
> 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.

> By the way, there's the following comment in StrategyInitalize.
>
> > * Initialize the shared buffer lookup hashtable.
> > *
> > * Since we can't tolerate running out of lookup table entries, we must be
> > * sure to specify an adequate table size here. The maximum steady-state
> > * usage is of course NBuffers entries, but BufferAlloc() tries to insert
> > * a new entry before deleting the old. In principle this could be
> > * happening in each partition concurrently, so we could need as many as
> > * NBuffers + NUM_BUFFER_PARTITIONS entries.
> > */
> > InitBufTable(NBuffers + NUM_BUFFER_PARTITIONS);
>
> "but BufferAlloc() tries to insert a new entry before deleting the
> old." gets false by this patch but still need that additional room for
> stashed entries. It seems like needing a fix.
>
>
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center