On Sun, Mar 13, 2022 at 3:27 PM Yura Sokolov <y.soko...@postgrespro.ru> wrote:
> В Вс, 13/03/2022 в 07:05 -0700, Zhihong Yu пишет: > > > > Hi, > > In the description: > > > > There is no need to hold both lock simultaneously. > > > > both lock -> both locks > > Thanks. > > > + * We also reset the usage_count since any recency of use of the old > > > > recency of use -> recent use > > Thanks. > > > +BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse) > > > > Later on, there is code: > > > > + reuse ? HASH_REUSE : HASH_REMOVE, > > > > Can flag (such as HASH_REUSE) be passed to BufTableDelete() instead of > bool ? That way, flag can be used directly in the above place. > > No. > BufTable* functions are created to abstract Buffer Table from dynahash. > Pass of HASH_REUSE directly will break abstraction. > > > + long nalloced; /* number of entries initially allocated > for > > > > nallocated isn't very long. I think it would be better to name the field > nallocated 'nallocated'. > > It is debatable. > Why not num_allocated? allocated_count? number_of_allocations? > Same points for nfree. > `nalloced` is recognizable and unambiguous. And there are a lot > of `*alloced` in the postgresql's source, so this one will not > be unusual. > > I don't see the need to make it longer. > > But if someone supports your point, I will not mind to changing > the name. > > > + sum += hashp->hctl->freeList[i].nalloced; > > + sum -= hashp->hctl->freeList[i].nfree; > > > > I think it would be better to calculate the difference between nalloced > and nfree first, then add the result to sum (to avoid overflow). > > Doesn't really matter much, because calculation must be valid > even if all nfree==0. > > I'd rather debate use of 'long' in dynahash at all: 'long' is > 32bit on 64bit Windows. It is better to use 'Size' here. > > But 'nelements' were 'long', so I didn't change things. I think > it is place for another patch. > > (On the other hand, dynahash with 2**31 elements is at least > 512GB RAM... we doubtfully trigger problem before OOM killer > came. Does Windows have an OOM killer?) > > > Subject: [PATCH 3/3] reduce memory allocation for non-partitioned > dynahash > > > > memory allocation -> memory allocations > > For each dynahash instance single allocation were reduced. > I think, 'memory allocation' is correct. > > Plural will be > reduce memory allocations for non-partitioned dynahashes > ie both 'allocations' and 'dynahashes'. > Am I wrong? > > Hi, bq. reduce memory allocation for non-partitioned dynahash It seems the following is clearer: reduce one memory allocation for every non-partitioned dynahash Cheers
