В Вс, 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?)